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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list