Re: [PATCH v2] settings: Resolve path if hostname is a sym-link

[Thomas Haller <thaller redhat com>]

On Mon, 2016-01-25 at 22:57 +0000, Joel Holdsworth wrote:
> If the hostname file is a symbolic link, follow it to find where the
> real file is located, otherwise g_file_set_contents will attempt to
> replace the link with a plain file.
> ---
>  src/settings/nm-settings.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)


Hi Joel,



> diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
> index f6f8c37..fc2eb29 100644
> --- a/src/settings/nm-settings.c
> +++ b/src/settings/nm-settings.c
> @@ -1532,11 +1532,11 @@ write_hostname (NMSettingsPrivate *priv,
> const char *hostname)
>       char *hostname_eol;
>       gboolean ret;
>       gs_free_error GError *error = NULL;
> -     const char *file = priv->hostname.file;
> +     char *file = g_strdup(priv->hostname.file), *link_path;

Whitespace between "g_strdup" and "(".


This now leaks @file.

Better do:

        const char *file = priv->hostname.file;
        gs_free char *link_path = NULL;


and later:

        if (   lstat (file, &file_stat) == 0
            && S_ISLNK  (file_stat.st_mode)
            && (link_path = g_file_read_link (file, NULL)))
            file = link_path;

which also avoid the additional copy.


>       gs_unref_variant GVariant *var = NULL;
> +     struct stat file_stat = { .st_mode = 0 };
>  #if HAVE_SELINUX
>       security_context_t se_ctx_prev = NULL, se_ctx = NULL;
> -     struct stat file_stat = { .st_mode = 0 };
>       mode_t st_mode = 0;
>  #endif
>  
> @@ -1554,6 +1554,16 @@ write_hostname (NMSettingsPrivate *priv, const
> char *hostname)
>               return !error;
>       }
>  



Anyway, why do we event want this? It's not clear to me that this is
desired behavior.

If NM is configured to overwrite /etc/hostname, with /etc/hostname
symlinking to somewhere else (e.g. /var/run/some-daemon/hostname), then
I don't think that NM should overwrite the file owned by "some-daemon"
but always rewrite /etc/hostname.

Note that for /etc/resolv.conf, NM only writes to
/var/run/NetworkManager/resolv.conf without redirecting the symlink in
/etc.
Maybe we should do something similar with /etc/hostname, but then it
seems to me that hostnamed is the future to manage the hostname.


ciao
Thomas

Attachment: signature.asc
Description: This is a digitally signed message part