[PATCH v2 0/2] qga: Ditch g_get_host_name()

[PATCH v2 0/2] qga: Ditch g_get_host_name()

[Michal Privoznik]

v2 of:

https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg04457.html

diff to v1:
- Move implementation out from qga/ to util/oslib-*

Michal Privoznik (2):
  util: Introduce qemu_get_host_name()
  qga: Use qemu_get_host_name() instead of g_get_host_name()

 include/qemu/osdep.h | 10 ++++++++++
 qga/commands.c       | 17 +++++++++++++----
 util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 13 +++++++++++++
 4 files changed, 68 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH v2 0/2] qga: Ditch g_get_host_name()

[Michal Privoznik]

v2 of:

https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg04457.html

diff to v1:
- Move implementation out from qga/ to util/oslib-*

Michal Privoznik (2):
  util: Introduce qemu_get_host_name()
  qga: Use qemu_get_host_name() instead of g_get_host_name()

 include/qemu/osdep.h | 10 ++++++++++
 qga/commands.c       | 17 +++++++++++++----
 util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 13 +++++++++++++
 4 files changed, 68 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Michal Privoznik]

This function offers operating system agnostic way to fetch host
name. It is implemented for both POSIX-like and Windows systems.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 include/qemu/osdep.h | 10 ++++++++++
 util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 13 +++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ff7c17b857..a795d46b28 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
 #endif
 }
 
+/**
+ * qemu_get_host_name:
+ * @errp: Error object
+ *
+ * Operating system agnostic way of querying host name.
+ *
+ * Returns allocated hostname (caller should free), NULL on failure.
+ */
+char *qemu_get_host_name(Error **errp);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 916f1be224..865a3d71a7 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -761,3 +761,35 @@ void sigaction_invoke(struct sigaction *action,
     }
     action->sa_sigaction(info->ssi_signo, &si, NULL);
 }
+
+#ifndef HOST_NAME_MAX
+# ifdef _POSIX_HOST_NAME_MAX
+#  define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
+# else
+#  define HOST_NAME_MAX 255
+# endif
+#endif
+
+char *qemu_get_host_name(Error **errp)
+{
+    long len = -1;
+    char *hostname;
+
+#ifdef _SC_HOST_NAME_MAX
+    len = sysconf(_SC_HOST_NAME_MAX);
+#endif /* _SC_HOST_NAME_MAX */
+
+    if (len < 0) {
+        len = HOST_NAME_MAX;
+    }
+
+    hostname = g_malloc0(len + 1);
+
+    if (gethostname(hostname, len) < 0) {
+        error_setg_errno(errp, errno,
+                         "cannot get hostname");
+        return NULL;
+    }
+
+    return hostname;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index e9b14ab178..3b49d27297 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -808,3 +808,16 @@ bool qemu_write_pidfile(const char *filename, Error **errp)
     }
     return true;
 }
+
+char *qemu_get_host_name(Error **errp)
+{
+    wchar_t tmp[MAX_COMPUTERNAME_LENGTH + 1];
+    DWORD size = G_N_ELEMENTS(tmp);
+
+    if (GetComputerNameW(tmp, &size) == 0) {
+        error_setg_win32(errp, GetLastError(), "failed close handle");
+        return NULL;
+    }
+
+    return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
+}
-- 
2.26.2

[PATCH v2 2/2] qga: Use qemu_get_host_name() instead of g_get_host_name()

[Michal Privoznik]

Problem with g_get_host_name() is that on the first call it saves
the hostname into a global variable and from then on, every
subsequent call returns the saved hostname. Even if the hostname
changes. This doesn't play nicely with guest agent, because if
the hostname is acquired before the guest is set up (e.g. on the
first boot, or before DHCP) we will report old, invalid hostname.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1845127

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 qga/commands.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index efc8b90281..d3fec807c1 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -515,11 +515,20 @@ int ga_parse_whence(GuestFileWhence *whence, Error **errp)
 GuestHostName *qmp_guest_get_host_name(Error **errp)
 {
     GuestHostName *result = NULL;
-    gchar const *hostname = g_get_host_name();
-    if (hostname != NULL) {
-        result = g_new0(GuestHostName, 1);
-        result->host_name = g_strdup(hostname);
+    g_autofree char *hostname = qemu_get_host_name(errp);
+
+    /*
+     * We want to avoid using g_get_host_name() because that
+     * caches the result and we wouldn't reflect changes in the
+     * host name.
+     */
+
+    if (!hostname) {
+        hostname = g_strdup("localhost");
     }
+
+    result = g_new0(GuestHostName, 1);
+    result->host_name = g_steal_pointer(&hostname);
     return result;
 }
 
-- 
2.26.2

Re: [PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Daniel P. Berrangé]

On Mon, Jun 22, 2020 at 07:26:44PM +0200, Michal Privoznik wrote:
> This function offers operating system agnostic way to fetch host
> name. It is implemented for both POSIX-like and Windows systems.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  include/qemu/osdep.h | 10 ++++++++++
>  util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   | 13 +++++++++++++
>  3 files changed, 55 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index ff7c17b857..a795d46b28 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
>  #endif
>  }
>  
> +/**
> + * qemu_get_host_name:
> + * @errp: Error object
> + *
> + * Operating system agnostic way of querying host name.
> + *
> + * Returns allocated hostname (caller should free), NULL on failure.
> + */
> +char *qemu_get_host_name(Error **errp);
> +
>  #endif
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 916f1be224..865a3d71a7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -761,3 +761,35 @@ void sigaction_invoke(struct sigaction *action,
>      }
>      action->sa_sigaction(info->ssi_signo, &si, NULL);
>  }
> +
> +#ifndef HOST_NAME_MAX
> +# ifdef _POSIX_HOST_NAME_MAX
> +#  define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
> +# else
> +#  define HOST_NAME_MAX 255
> +# endif
> +#endif
> +
> +char *qemu_get_host_name(Error **errp)
> +{
> +    long len = -1;
> +    char *hostname;
> +
> +#ifdef _SC_HOST_NAME_MAX
> +    len = sysconf(_SC_HOST_NAME_MAX);
> +#endif /* _SC_HOST_NAME_MAX */
> +
> +    if (len < 0) {
> +        len = HOST_NAME_MAX;
> +    }
> +
> +    hostname = g_malloc0(len + 1);

Nitpick, generally qemu prefers g_new0

> +
> +    if (gethostname(hostname, len) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "cannot get hostname");
> +        return NULL;
> +    }

According to my man page, it is undefined by POSIX whether there's a
trailing NUL when  hostname exceeds the buffer, so the paranoid thing
todo is to add

  hostname[len] = '\0';

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Philippe Mathieu-Daudé]

On 6/22/20 7:26 PM, Michal Privoznik wrote:
> This function offers operating system agnostic way to fetch host
> name. It is implemented for both POSIX-like and Windows systems.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  include/qemu/osdep.h | 10 ++++++++++
>  util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   | 13 +++++++++++++
>  3 files changed, 55 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index ff7c17b857..a795d46b28 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
>  #endif
>  }
>  
> +/**
> + * qemu_get_host_name:
> + * @errp: Error object
> + *
> + * Operating system agnostic way of querying host name.
> + *
> + * Returns allocated hostname (caller should free), NULL on failure.

free -> g_free?

> + */
> +char *qemu_get_host_name(Error **errp);
> +
>  #endif
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 916f1be224..865a3d71a7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -761,3 +761,35 @@ void sigaction_invoke(struct sigaction *action,
>      }
>      action->sa_sigaction(info->ssi_signo, &si, NULL);
>  }
> +
> +#ifndef HOST_NAME_MAX
> +# ifdef _POSIX_HOST_NAME_MAX
> +#  define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
> +# else
> +#  define HOST_NAME_MAX 255
> +# endif
> +#endif
> +
> +char *qemu_get_host_name(Error **errp)
> +{
> +    long len = -1;
> +    char *hostname;
> +
> +#ifdef _SC_HOST_NAME_MAX
> +    len = sysconf(_SC_HOST_NAME_MAX);
> +#endif /* _SC_HOST_NAME_MAX */
> +
> +    if (len < 0) {
> +        len = HOST_NAME_MAX;
> +    }
> +
> +    hostname = g_malloc0(len + 1);
> +
> +    if (gethostname(hostname, len) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "cannot get hostname");

Missing:

           g_free(hostname);

> +        return NULL;
> +    }
> +
> +    return hostname;
> +}
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index e9b14ab178..3b49d27297 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -808,3 +808,16 @@ bool qemu_write_pidfile(const char *filename, Error **errp)
>      }
>      return true;
>  }
> +
> +char *qemu_get_host_name(Error **errp)
> +{
> +    wchar_t tmp[MAX_COMPUTERNAME_LENGTH + 1];
> +    DWORD size = G_N_ELEMENTS(tmp);
> +
> +    if (GetComputerNameW(tmp, &size) == 0) {
> +        error_setg_win32(errp, GetLastError(), "failed close handle");
> +        return NULL;
> +    }
> +
> +    return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
> +}
> 

Re: [PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Daniel P. Berrangé]

On Mon, Jun 22, 2020 at 07:46:08PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/22/20 7:26 PM, Michal Privoznik wrote:
> > This function offers operating system agnostic way to fetch host
> > name. It is implemented for both POSIX-like and Windows systems.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  include/qemu/osdep.h | 10 ++++++++++
> >  util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
> >  util/oslib-win32.c   | 13 +++++++++++++
> >  3 files changed, 55 insertions(+)
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index ff7c17b857..a795d46b28 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
> >  #endif
> >  }
> >  
> > +/**
> > + * qemu_get_host_name:
> > + * @errp: Error object
> > + *
> > + * Operating system agnostic way of querying host name.
> > + *
> > + * Returns allocated hostname (caller should free), NULL on failure.
> 
> free -> g_free?

free & g_free are guaranteed interchangable, given our min glib2 version.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Michal Privoznik]

On 6/22/20 7:38 PM, Daniel P. Berrangé wrote:
> On Mon, Jun 22, 2020 at 07:26:44PM +0200, Michal Privoznik wrote:
>> This function offers operating system agnostic way to fetch host
>> name. It is implemented for both POSIX-like and Windows systems.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   include/qemu/osdep.h | 10 ++++++++++
>>   util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
>>   util/oslib-win32.c   | 13 +++++++++++++
>>   3 files changed, 55 insertions(+)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index ff7c17b857..a795d46b28 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
>>   #endif
>>   }
>>   
>> +/**
>> + * qemu_get_host_name:
>> + * @errp: Error object
>> + *
>> + * Operating system agnostic way of querying host name.
>> + *
>> + * Returns allocated hostname (caller should free), NULL on failure.
>> + */
>> +char *qemu_get_host_name(Error **errp);
>> +
>>   #endif
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index 916f1be224..865a3d71a7 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -761,3 +761,35 @@ void sigaction_invoke(struct sigaction *action,
>>       }
>>       action->sa_sigaction(info->ssi_signo, &si, NULL);
>>   }
>> +
>> +#ifndef HOST_NAME_MAX
>> +# ifdef _POSIX_HOST_NAME_MAX
>> +#  define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
>> +# else
>> +#  define HOST_NAME_MAX 255
>> +# endif
>> +#endif
>> +
>> +char *qemu_get_host_name(Error **errp)
>> +{
>> +    long len = -1;
>> +    char *hostname;
>> +
>> +#ifdef _SC_HOST_NAME_MAX
>> +    len = sysconf(_SC_HOST_NAME_MAX);
>> +#endif /* _SC_HOST_NAME_MAX */
>> +
>> +    if (len < 0) {
>> +        len = HOST_NAME_MAX;
>> +    }
>> +
>> +    hostname = g_malloc0(len + 1);
> 
> Nitpick, generally qemu prefers g_new0
> 
>> +
>> +    if (gethostname(hostname, len) < 0) {
>> +        error_setg_errno(errp, errno,
>> +                         "cannot get hostname");
>> +        return NULL;
>> +    }
> 
> According to my man page, it is undefined by POSIX whether there's a
> trailing NUL when  hostname exceeds the buffer, so the paranoid thing
> todo is to add
> 
>    hostname[len] = '\0';

Isn't this guaranteed by allocating len + 1 bytes? I mean, g_malloc0() 
and g_new0() will memset() the memory to zero. And since I tell 
gethostname() the buf is only len bytes long I am guaranteed to have 0 
at the end of it, aren't I? Maybe I should put a comment just before 
g_malloc0() or g_new0() that documents this thought.

Michal

Re: [PATCH v2 1/2] util: Introduce qemu_get_host_name()

[Daniel P. Berrangé]

On Mon, Jun 22, 2020 at 07:53:40PM +0200, Michal Privoznik wrote:
> On 6/22/20 7:38 PM, Daniel P. Berrangé wrote:
> > On Mon, Jun 22, 2020 at 07:26:44PM +0200, Michal Privoznik wrote:
> > > This function offers operating system agnostic way to fetch host
> > > name. It is implemented for both POSIX-like and Windows systems.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   include/qemu/osdep.h | 10 ++++++++++
> > >   util/oslib-posix.c   | 32 ++++++++++++++++++++++++++++++++
> > >   util/oslib-win32.c   | 13 +++++++++++++
> > >   3 files changed, 55 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index ff7c17b857..a795d46b28 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -607,4 +607,14 @@ static inline void qemu_reset_optind(void)
> > >   #endif
> > >   }
> > > +/**
> > > + * qemu_get_host_name:
> > > + * @errp: Error object
> > > + *
> > > + * Operating system agnostic way of querying host name.
> > > + *
> > > + * Returns allocated hostname (caller should free), NULL on failure.
> > > + */
> > > +char *qemu_get_host_name(Error **errp);
> > > +
> > >   #endif
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index 916f1be224..865a3d71a7 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -761,3 +761,35 @@ void sigaction_invoke(struct sigaction *action,
> > >       }
> > >       action->sa_sigaction(info->ssi_signo, &si, NULL);
> > >   }
> > > +
> > > +#ifndef HOST_NAME_MAX
> > > +# ifdef _POSIX_HOST_NAME_MAX
> > > +#  define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
> > > +# else
> > > +#  define HOST_NAME_MAX 255
> > > +# endif
> > > +#endif
> > > +
> > > +char *qemu_get_host_name(Error **errp)
> > > +{
> > > +    long len = -1;
> > > +    char *hostname;
> > > +
> > > +#ifdef _SC_HOST_NAME_MAX
> > > +    len = sysconf(_SC_HOST_NAME_MAX);
> > > +#endif /* _SC_HOST_NAME_MAX */
> > > +
> > > +    if (len < 0) {
> > > +        len = HOST_NAME_MAX;
> > > +    }
> > > +
> > > +    hostname = g_malloc0(len + 1);
> > 
> > Nitpick, generally qemu prefers g_new0
> > 
> > > +
> > > +    if (gethostname(hostname, len) < 0) {
> > > +        error_setg_errno(errp, errno,
> > > +                         "cannot get hostname");
> > > +        return NULL;
> > > +    }
> > 
> > According to my man page, it is undefined by POSIX whether there's a
> > trailing NUL when  hostname exceeds the buffer, so the paranoid thing
> > todo is to add
> > 
> >    hostname[len] = '\0';
> 
> Isn't this guaranteed by allocating len + 1 bytes? I mean, g_malloc0() and
> g_new0() will memset() the memory to zero. And since I tell gethostname()
> the buf is only len bytes long I am guaranteed to have 0 at the end of it,
> aren't I? Maybe I should put a comment just before g_malloc0() or g_new0()
> that documents this thought.

Oh right, yes, I'm mis-reading the code.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|