[libvirt] [PATCH v3 0/4] Replace VIR_ERROR with standard vir*Error in state driver init
Replace VIR_ERROR logging macros for error reporting in driver startup routines with vir*Error function. Don't use virStrerror and remove ebuf. Link to task: http://wiki.libvirt.org/page/BiteSizedTasks#Replace_VIR_ERROR_with_standard_... Jovanka Gulicoska (4): uml: Replace VIR_ERROR with standard vir*Error in state driver init qemu: Replace VIR_ERROR with standard vir*Error in state driver init xen: Replace VIR_ERROR with standard vir*Error in state driver init node_device: Replace VIR_ERROR with standard vir*Error in state driver init src/node_device/node_device_hal.c | 26 +++++++++++------- src/node_device/node_device_udev.c | 49 ++++++++++++++++++++-------------- src/qemu/qemu_driver.c | 54 +++++++++++++++++++++++--------------- src/uml/uml_driver.c | 20 ++++++-------- src/xen/xen_driver.c | 5 ++-- 5 files changed, 90 insertions(+), 64 deletions(-) -- 2.5.5
--- src/uml/uml_driver.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a674c12..8dd54d7 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -188,8 +188,8 @@ umlAutostartDomain(virDomainObjPtr vm, ret = umlStartVMDaemon(data->conn, data->driver, vm, false); virDomainAuditStart(vm, "booted", ret >= 0); if (ret < 0) { - VIR_ERROR(_("Failed to autostart VM '%s': %s"), - vm->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart VM '%s': %s"), + vm->def->name, virGetLastErrorMessage()); } else { virObjectEventPtr event = virDomainEventLifecycleNewFromObj(vm, @@ -535,15 +535,13 @@ umlStateInitialize(bool privileged, goto error; if ((uml_driver->inotifyFD = inotify_init()) < 0) { - VIR_ERROR(_("cannot initialize inotify")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize inotify")); goto error; } if (virFileMakePath(uml_driver->monitorDir) < 0) { - char ebuf[1024]; - VIR_ERROR(_("Failed to create monitor directory %s: %s"), - uml_driver->monitorDir, - virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, _("Failed to create monitor directory %s"), + uml_driver->monitorDir); goto error; } @@ -551,10 +549,8 @@ umlStateInitialize(bool privileged, if (inotify_add_watch(uml_driver->inotifyFD, uml_driver->monitorDir, IN_CREATE | IN_MODIFY | IN_DELETE) < 0) { - char ebuf[1024]; - VIR_ERROR(_("Failed to create inotify watch on %s: %s"), - uml_driver->monitorDir, - virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, _("Failed to create inotify watch on %s"), + uml_driver->monitorDir); goto error; } @@ -582,7 +578,7 @@ umlStateInitialize(bool privileged, return 0; out_of_memory: - VIR_ERROR(_("umlStartup: out of memory")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("umlStartup: out of memory")); error: VIR_FREE(userdir); -- 2.5.5
--- src/qemu/qemu_driver.c | 54 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 10d3e3d..85a116a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -290,15 +290,17 @@ qemuAutostartDomain(virDomainObjPtr vm, if (vm->autostart && !virDomainObjIsActive(vm)) { if (qemuProcessBeginJob(data->driver, vm) < 0) { - VIR_ERROR(_("Failed to start job on VM '%s': %s"), - vm->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to start job on VM '%s': %s"), + vm->def->name, virGetLastErrorMessage()); goto cleanup; } if (qemuDomainObjStart(data->conn, data->driver, vm, flags, QEMU_ASYNC_JOB_START) < 0) { - VIR_ERROR(_("Failed to autostart VM '%s': %s"), - vm->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart VM '%s': %s"), + vm->def->name, virGetLastErrorMessage()); } qemuProcessEndJob(data->driver, vm); @@ -450,7 +452,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) return 0; error: - VIR_ERROR(_("Failed to initialize security drivers")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to initialize security drivers")); virObjectUnref(stack); virObjectUnref(mgr); virObjectUnref(cfg); @@ -471,7 +474,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, virDomainSnapshotDefPtr def = NULL; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotObjPtr current = NULL; - char ebuf[1024]; unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); @@ -481,8 +483,10 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, virObjectLock(vm); if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { - VIR_ERROR(_("Failed to allocate memory for snapshot directory for domain %s"), - vm->def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to allocate memory for " + "snapshot directory for domain %s"), + vm->def->name); goto cleanup; } @@ -494,9 +498,10 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, if (!(dir = opendir(snapDir))) { if (errno != ENOENT) - VIR_ERROR(_("Failed to open snapshot directory %s for domain %s: %s"), - snapDir, vm->def->name, - virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, + _("Failed to open snapshot directory %s " + "for domain %s"), + snapDir, vm->def->name); goto cleanup; } @@ -509,14 +514,16 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, VIR_INFO("Loading snapshot file '%s'", entry->d_name); if (virAsprintf(&fullpath, "%s/%s", snapDir, entry->d_name) < 0) { - VIR_ERROR(_("Failed to allocate memory for path")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to allocate memory for path")); continue; } if (virFileReadAll(fullpath, 1024*1024*1, &xmlStr) < 0) { /* Nothing we can do here, skip this one */ - VIR_ERROR(_("Failed to read snapshot file %s: %s"), fullpath, - virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, + _("Failed to read snapshot file %s"), + fullpath); VIR_FREE(fullpath); continue; } @@ -526,8 +533,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ - VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), - fullpath); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse snapshot XML from file '%s'"), + fullpath); VIR_FREE(fullpath); VIR_FREE(xmlStr); continue; @@ -546,17 +554,21 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, VIR_FREE(xmlStr); } if (direrr < 0) - VIR_ERROR(_("Failed to fully read directory %s"), snapDir); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to fully read directory %s"), + snapDir); if (vm->current_snapshot != current) { - VIR_ERROR(_("Too many snapshots claiming to be current for domain %s"), - vm->def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Too many snapshots claiming to be current for domain %s"), + vm->def->name); vm->current_snapshot = NULL; } if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0) - VIR_ERROR(_("Snapshots have inconsistent relations for domain %s"), - vm->def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Snapshots have inconsistent relations for domain %s"), + vm->def->name); /* FIXME: qemu keeps internal track of snapshots. We can get access * to this info via the "info snapshots" monitor command for running -- 2.5.5
--- src/xen/xen_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 3f5d80d..04c0809 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -407,7 +407,6 @@ static virDrvOpenStatus xenUnifiedConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) { xenUnifiedPrivatePtr priv; - char ebuf[1024]; /* * Only the libvirtd instance can open this driver. @@ -532,8 +531,8 @@ xenUnifiedConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int f goto error; if (virFileMakePath(priv->saveDir) < 0) { - VIR_ERROR(_("Errored to create save dir '%s': %s"), priv->saveDir, - virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, _("Errored to create save dir '%s'"), + priv->saveDir); goto error; } -- 2.5.5
--- src/node_device/node_device_hal.c | 26 +++++++++++++------- src/node_device/node_device_udev.c | 49 +++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 6ddfad0..7a9c3d9 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -641,24 +641,29 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, dbus_error_init(&err); if (!(sysbus = virDBusGetSystemBus())) { - VIR_ERROR(_("DBus not available, disabling HAL driver: %s"), - virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("DBus not available, disabling HAL driver: %s"), + virGetLastErrorMessage()); ret = 0; goto failure; } hal_ctx = libhal_ctx_new(); if (hal_ctx == NULL) { - VIR_ERROR(_("libhal_ctx_new returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libhal_ctx_new returned NULL")); goto failure; } if (!libhal_ctx_set_dbus_connection(hal_ctx, sysbus)) { - VIR_ERROR(_("libhal_ctx_set_dbus_connection failed")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libhal_ctx_set_dbus_connection failed")); goto failure; } if (!libhal_ctx_init(hal_ctx, &err)) { - VIR_ERROR(_("libhal_ctx_init failed, haldaemon is probably not running")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libhal_ctx_init failed, haldaemon " + "is probably not running")); /* We don't want to show a fatal error here, otherwise entire libvirtd shuts down when hald isn't running */ @@ -683,13 +688,15 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, !libhal_ctx_set_device_lost_capability(hal_ctx, device_cap_lost) || !libhal_ctx_set_device_property_modified(hal_ctx, device_prop_modified) || !libhal_device_property_watch_all(hal_ctx, &err)) { - VIR_ERROR(_("setting up HAL callbacks failed")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("setting up HAL callbacks failed")); goto failure; } udi = libhal_get_all_devices(hal_ctx, &num_devs, &err); if (udi == NULL) { - VIR_ERROR(_("libhal_get_all_devices failed")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libhal_get_all_devices failed")); goto failure; } for (i = 0; i < num_devs; i++) { @@ -702,7 +709,7 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, failure: if (dbus_error_is_set(&err)) { - VIR_ERROR(_("%s: %s"), err.name, err.message); + virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: %s"), err.name, err.message); dbus_error_free(&err); } virNodeDeviceObjListFree(&driver->devs); @@ -753,7 +760,8 @@ nodeStateReload(void) dbus_error_init(&err); udi = libhal_get_all_devices(hal_ctx, &num_devs, &err); if (udi == NULL) { - VIR_ERROR(_("libhal_get_all_devices failed")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libhal_get_all_devices failed")); return -1; } for (i = 0; i < num_devs; i++) { diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7d111c4..3315153 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -66,7 +66,8 @@ static int udevStrToLong_ull(char const *s, ret = virStrToLong_ull(s, end_ptr, base, result); if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), s); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to unsigned long long"), s); } else { VIR_DEBUG("Converted '%s' to unsigned long %llu", s, *result); } @@ -84,7 +85,8 @@ static int udevStrToLong_ui(char const *s, ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to unsigned int"), s); } else { VIR_DEBUG("Converted '%s' to unsigned int %u", s, *result); } @@ -101,7 +103,8 @@ static int udevStrToLong_i(char const *s, ret = virStrToLong_i(s, end_ptr, base, result); if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), s); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to int"), s); } else { VIR_DEBUG("Converted '%s' to int %u", s, *result); } @@ -717,8 +720,9 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, filename = last_component(def->sysfs_path); if (!(str = STRSKIP(filename, "host"))) { - VIR_ERROR(_("SCSI host found, but its udev name '%s' does " - "not begin with 'host'"), filename); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("SCSI host found, but its udev name '%s' does " + "not begin with 'host'"), filename); goto out; } @@ -875,8 +879,9 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, out: if (ret != 0) { - VIR_ERROR(_("Failed to process SCSI device with sysfs path '%s'"), - def->sysfs_path); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to process SCSI device with sysfs path '%s'"), + def->sysfs_path); } return ret; } @@ -1293,7 +1298,8 @@ static int udevGetDeviceDetails(struct udev_device *device, ret = udevProcessSCSIGeneric(device, def); break; default: - VIR_ERROR(_("Unknown device type %d"), def->caps->data.type); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown device type %d"), def->caps->data.type); ret = -1; break; } @@ -1459,7 +1465,8 @@ static int udevEnumerateDevices(struct udev *udev) ret = udev_enumerate_scan_devices(udev_enumerate); if (0 != ret) { - VIR_ERROR(_("udev scan devices returned %d"), ret); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("udev scan devices returned %d"), ret); goto out; } @@ -1539,14 +1546,16 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, nodeDeviceLock(); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { - VIR_ERROR(_("File descriptor returned by udev %d does not " - "match node device file descriptor %d"), fd, udev_fd); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("File descriptor returned by udev %d does not " + "match node device file descriptor %d"), fd, udev_fd); goto out; } device = udev_monitor_receive_device(udev_monitor); if (device == NULL) { - VIR_ERROR(_("udev_monitor_receive_device returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_receive_device returned NULL")); goto out; } @@ -1584,8 +1593,9 @@ udevGetDMIData(virNodeDevCapDataPtr data) if (device == NULL) { device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK); if (device == NULL) { - VIR_ERROR(_("Failed to get udev device for syspath '%s' or '%s'"), - DMI_DEVPATH, DMI_DEVPATH_FALLBACK); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get udev device for syspath '%s' or '%s'"), + DMI_DEVPATH, DMI_DEVPATH_FALLBACK); goto out; } } @@ -1691,9 +1701,8 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) * situation, but a non-privileged user won't benefit much * from udev in the first place. */ if (errno != ENOENT && (privileged || errno != EACCES)) { - char ebuf[256]; - VIR_ERROR(_("Failed to initialize libpciaccess: %s"), - virStrerror(pciret, ebuf, sizeof(ebuf))); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to initialize libpciaccess: %s")); return -1; } } @@ -1721,7 +1730,8 @@ static int nodeStateInitialize(bool privileged, } if (virMutexInit(&driver->lock) < 0) { - VIR_ERROR(_("Failed to initialize mutex for driver")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to initialize mutex for driver")); VIR_FREE(priv); VIR_FREE(driver); return -1; @@ -1747,7 +1757,8 @@ static int nodeStateInitialize(bool privileged, priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { - VIR_ERROR(_("udev_monitor_new_from_netlink returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_new_from_netlink returned NULL")); goto out_unlock; } -- 2.5.5
On 06/07/2016 11:59 AM, Jovanka Gulicoska wrote:
--- src/node_device/node_device_hal.c | 26 +++++++++++++------- src/node_device/node_device_udev.c | 49 +++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 28 deletions(-)
I pushed patches 1-3, thanks! This patch needs to be rebased on master, since node_device_udev had a bunch of cleanups recently. There's also one issue I noticed:
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7d111c4..3315153 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
@@ -1691,9 +1701,8 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) * situation, but a non-privileged user won't benefit much * from udev in the first place. */ if (errno != ENOENT && (privileged || errno != EACCES)) { - char ebuf[256]; - VIR_ERROR(_("Failed to initialize libpciaccess: %s"), - virStrerror(pciret, ebuf, sizeof(ebuf))); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to initialize libpciaccess: %s")); return -1; } }
I think you forgot to preserve the virStrerror call here, otherwise the string format is incorrect, with the trailing ": %s" Thanks, Cole
participants (2)
-
Cole Robinson -
Jovanka Gulicoska