[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Improve error reporting for virConnectGetHostname calls



Cole Robinson wrote:
> All drivers have copy + pasted inadequate error reporting which wraps
> util.c:virGetHostname. Move all error reporting to this function, and improve
> what we report.

Overall, a good idea, I think.  A few nits below.

> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 03d091a..058e684 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -939,9 +939,8 @@ static struct qemud_server *qemudNetworkInit(struct qemud_server *server) {
>          if (!mdns_name) {
>              char groupname[64], *localhost, *tmp;
>              /* Extract the host part of the potentially FQDN */
> -            localhost = virGetHostname();
> +            localhost = virGetHostname(NULL);
>              if (localhost == NULL) {
> -                virReportOOMError(NULL);
>                  goto cleanup;
>              }

Drop the braces here as well.

>              if ((tmp = strchr(localhost, '.')))
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index ef97364..73f1c8e 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -2058,16 +2058,8 @@ cleanup:
>  
>  static char *lxcGetHostname (virConnectPtr conn)
>  {
> -    char *result;
> -
> -    result = virGetHostname();
> -    if (result == NULL) {
> -        virReportSystemError (conn, errno,
> -                              "%s", _("failed to determine host name"));
> -        return NULL;
> -    }
>      /* Caller frees this string. */
> -    return result;
> +    return virGetHostname(conn);
>  }

Just get rid of the whole function and call virGetHostname() directly where
lxcGetHostname() would have been called.

>  
>  static int lxcFreezeContainer(lxc_driver_t *driver, virDomainObjPtr vm)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a3beedb..86546e5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2623,16 +2623,8 @@ cleanup:
>  static char *
>  qemudGetHostname (virConnectPtr conn)
>  {
> -    char *result;
> -
> -    result = virGetHostname();
> -    if (result == NULL) {
> -        virReportSystemError (conn, errno,
> -                              "%s", _("failed to determine host name"));
> -        return NULL;
> -    }
>      /* Caller frees this string. */
> -    return result;
> +    return virGetHostname(conn);
>  }

Same here.

>  
>  static int qemudListDomains(virConnectPtr conn, int *ids, int nids) {
> @@ -6283,9 +6275,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
>          if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
>  
>          /* Get hostname */
> -        if ((hostname = virGetHostname()) == NULL) {
> -            virReportSystemError (dconn, errno,
> -                                  "%s", _("failed to determine host name"));
> +        if ((hostname = virGetHostname(dconn)) == NULL) {
>              goto cleanup;
>          }
>  
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 36f8158..4a081be 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -994,16 +994,8 @@ static int testGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>  static char *testGetHostname (virConnectPtr conn)
>  {
> -    char *result;
> -
> -    result = virGetHostname();
> -    if (result == NULL) {
> -        virReportSystemError(conn, errno,
> -                             "%s", _("cannot lookup hostname"));
> -        return NULL;
> -    }
>      /* Caller frees this string. */
> -    return result;
> +    return virGetHostname(conn);
>  }

Same here.

>  
>  static int testGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED,
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 9a7fe42..cd92a6b 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -1131,16 +1131,8 @@ cleanup:
>  static char *
>  umlGetHostname (virConnectPtr conn)
>  {
> -    char *result;
> -
> -    result = virGetHostname();
> -    if (result == NULL) {
> -        virReportSystemError(conn, errno,
> -                             "%s", _("cannot lookup hostname"));
> -        return NULL;
> -    }
>      /* Caller frees this string. */
> -    return result;
> +    return virGetHostname(conn);
>  }

Same here.

>  
>  static int umlListDomains(virConnectPtr conn, int *ids, int nids) {
> diff --git a/src/util/util.c b/src/util/util.c
> index 98f8a14..49eac6d 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -1804,30 +1804,42 @@ int virDiskNameToIndex(const char *name) {
>  #define AI_CANONIDN 0
>  #endif
>  
> -char *virGetHostname(void)
> +char *virGetHostname(virConnectPtr conn)

To be honest, I'm not sure we even need to add the "conn" parameter here.  I
seem to remember DanB saying that it was only needed in very few circumstances
anymore.  Can't we just call "virReportSystemError(NULL)", etc?  That keeps this
function signature a bit simpler.  (It's not a deal-breaker in any case, just
nice to not have to require this parameter)

>  {
>      int r;
>      char hostname[HOST_NAME_MAX+1], *result;
>      struct addrinfo hints, *info;
>  
>      r = gethostname (hostname, sizeof(hostname));
> -    if (r == -1)
> +    if (r == -1) {
> +        virReportSystemError (conn, errno,
> +                              "%s", _("failed to determine host name"));
>          return NULL;
> +    }
>      NUL_TERMINATE(hostname);
>  
>      memset(&hints, 0, sizeof(hints));
>      hints.ai_flags = AI_CANONNAME|AI_CANONIDN;
>      hints.ai_family = AF_UNSPEC;
>      r = getaddrinfo(hostname, NULL, &hints, &info);
> -    if (r != 0)
> +    if (r != 0) {
> +        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                    _("getaddrinfo failed for '%s': %s"),
> +                    hostname, gai_strerror(r));
>          return NULL;
> +    }
>      if (info->ai_canonname == NULL) {
> +        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                    "%s", _("could not determine canonical host name"));
>          freeaddrinfo(info);
>          return NULL;
>      }
>  
>      /* Caller frees this string. */
>      result = strdup (info->ai_canonname);
> +    if (!result)
> +        virReportOOMError(conn);
> +
>      freeaddrinfo(info);
>      return result;
>  }
> diff --git a/src/util/util.h b/src/util/util.h
> index 8679636..85d5488 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -222,7 +222,7 @@ static inline int getuid (void) { return 0; }
>  static inline int getgid (void) { return 0; }
>  #endif
>  
> -char *virGetHostname(void);
> +char *virGetHostname(virConnectPtr conn);
>  
>  int virKillProcess(pid_t pid, int sig);
>  
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 4f43901..2a17946 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -594,17 +594,8 @@ static int vboxGetVersion(virConnectPtr conn, unsigned long *version) {
>  }
>  
>  static char *vboxGetHostname(virConnectPtr conn) {
> -    char *hostname;
> -
>      /* the return string should be freed by caller */
> -    hostname = virGetHostname();
> -    if (hostname == NULL) {
> -        vboxError(conn, VIR_ERR_INTERNAL_ERROR,"%s",
> -                  "failed to determine host name");
> -        return NULL;
> -    }
> -
> -    return hostname;
> +    return virGetHostname(conn);
>  }

Again, drop the wrapper function.

>  
>  static int vboxGetMaxVcpus(virConnectPtr conn, const char *type ATTRIBUTE_UNUSED) {
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index f2744b0..0818cd3 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -484,16 +484,8 @@ xenUnifiedGetVersion (virConnectPtr conn, unsigned long *hvVer)
>  static char *
>  xenUnifiedGetHostname (virConnectPtr conn)
>  {
> -    char *result;
> -
> -    result = virGetHostname();
> -    if (result == NULL) {
> -        virReportSystemError(conn, errno,
> -                             "%s", _("cannot lookup hostname"));
> -        return NULL;
> -    }
>      /* Caller frees this string. */
> -    return result;
> +    return virGetHostname(conn);
>  }

Drop the wrapper.

>  
>  static int
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index d3ab019..6d1d851 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -4347,9 +4347,8 @@ xenDaemonDomainMigratePrepare (virConnectPtr dconn,
>       * deallocates this string.
>       */
>      if (uri_in == NULL) {
> -        *uri_out = virGetHostname();
> +        *uri_out = virGetHostname(dconn);
>          if (*uri_out == NULL) {
> -            virReportOOMError(dconn);
>              return -1;
>          }
>      }

Drop the braces.

> diff --git a/tools/virsh.c b/tools/virsh.c
> index 6b93405..3c668da 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -524,7 +524,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom)
>      char *thatHost = NULL;
>      char *thisHost = NULL;
>  
> -    if (!(thisHost = virGetHostname())) {
> +    if (!(thisHost = virGetHostname(ctl->conn))) {
>          vshError(ctl, "%s", _("Failed to get local hostname"));
>          goto cleanup;
>      }


-- 
Chris Lalancette


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]