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

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

[Michal Privoznik]

v3 of:

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

diff to v2:
- don't leak @hostname in util/oslib-posix.c:qemu_get_host_name()
- document why we are allocating one byte more than needed
- switch to g_new0() from g_malloc0().

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   | 35 +++++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 13 +++++++++++++
 4 files changed, 71 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH v3 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   | 35 +++++++++++++++++++++++++++++++++++
 util/oslib-win32.c   | 13 +++++++++++++
 3 files changed, 58 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..3997ee0442 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -761,3 +761,38 @@ 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;
+    g_autofree char *hostname = NULL;
+
+#ifdef _SC_HOST_NAME_MAX
+    len = sysconf(_SC_HOST_NAME_MAX);
+#endif /* _SC_HOST_NAME_MAX */
+
+    if (len < 0) {
+        len = HOST_NAME_MAX;
+    }
+
+    /* Unfortunately, gethostname() below does not guarantee a
+     * NULL terminated string. Therefore, allocate one byte more
+     * to be sure. */
+    hostname = g_new0(char, len + 1);
+
+    if (gethostname(hostname, len) < 0) {
+        error_setg_errno(errp, errno,
+                         "cannot get hostname");
+        return NULL;
+    }
+
+    return g_steal_pointer(&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

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

[Philippe Mathieu-Daudé]

On 6/22/20 8:19 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>

Thanks for the updates!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  include/qemu/osdep.h | 10 ++++++++++
>  util/oslib-posix.c   | 35 +++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   | 13 +++++++++++++
>  3 files changed, 58 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..3997ee0442 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -761,3 +761,38 @@ 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;
> +    g_autofree char *hostname = NULL;
> +
> +#ifdef _SC_HOST_NAME_MAX
> +    len = sysconf(_SC_HOST_NAME_MAX);
> +#endif /* _SC_HOST_NAME_MAX */
> +
> +    if (len < 0) {
> +        len = HOST_NAME_MAX;
> +    }
> +
> +    /* Unfortunately, gethostname() below does not guarantee a
> +     * NULL terminated string. Therefore, allocate one byte more
> +     * to be sure. */
> +    hostname = g_new0(char, len + 1);
> +
> +    if (gethostname(hostname, len) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "cannot get hostname");
> +        return NULL;
> +    }
> +
> +    return g_steal_pointer(&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 v3 1/2] util: Introduce qemu_get_host_name()

[Daniel P. Berrangé]

On Mon, Jun 22, 2020 at 08:19:35PM +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   | 35 +++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   | 13 +++++++++++++
>  3 files changed, 58 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH v3 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 v3 2/2] qga: Use qemu_get_host_name() instead of g_get_host_name()

[Daniel P. Berrangé]

On Mon, Jun 22, 2020 at 08:19:36PM +0200, Michal Privoznik wrote:
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v3 0/2] qga: Ditch g_get_host_name()

[Michal Privoznik]

On 6/22/20 8:19 PM, Michal Privoznik wrote:
> v3 of:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg06913.html
> 
> diff to v2:
> - don't leak @hostname in util/oslib-posix.c:qemu_get_host_name()
> - document why we are allocating one byte more than needed
> - switch to g_new0() from g_malloc0().
> 
> 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   | 35 +++++++++++++++++++++++++++++++++++
>   util/oslib-win32.c   | 13 +++++++++++++
>   4 files changed, 71 insertions(+), 4 deletions(-)
> 

Ping? How can I get these merged please?

Michal

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

[Philippe Mathieu-Daudé]

Ping^2?

On 7/10/20 11:43 AM, Michal Privoznik wrote:
> On 6/22/20 8:19 PM, Michal Privoznik wrote:
>> v3 of:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg06913.html
>>
>> diff to v2:
>> - don't leak @hostname in util/oslib-posix.c:qemu_get_host_name()
>> - document why we are allocating one byte more than needed
>> - switch to g_new0() from g_malloc0().
>>
>> 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   | 35 +++++++++++++++++++++++++++++++++++
>>   util/oslib-win32.c   | 13 +++++++++++++
>>   4 files changed, 71 insertions(+), 4 deletions(-)
>>
> 
> Ping? How can I get these merged please?
> 
> Michal
> 
>