[libvirt] [PATCH v2 0/8] 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. Link to task: http://wiki.libvirt.org/page/BiteSizedTasks#Replace_VIR_ERROR_with_standard_... Jovanka Gulicoska (8): uml: Replace VIR_ERROR with standard vir*Error in state driver init xen: Replace VIR_ERROR with standard vir*Error in state driver init bhyve: Replace VIR_ERROR with standard vir*Error in state driver init libxl: Replace VIR_ERROR with standard vir*Error in state driver init node_device: Replace VIR_ERROR with standard vir*Error in state driver init nwfilter: Replace VIR_ERROR with standard vir*Error in state driver init qemu: Replace VIR_ERROR with standard vir*Error in state driver init storage: Replace VIR_ERROR with standard vir*Error in state driver init src/bhyve/bhyve_driver.c | 4 +-- src/libxl/libxl_driver.c | 6 ++--- src/node_device/node_device_hal.c | 24 ++++++++++------- src/node_device/node_device_udev.c | 45 +++++++++++++++++++------------ src/nwfilter/nwfilter_driver.c | 5 ++-- src/qemu/qemu_driver.c | 55 ++++++++++++++++++++++++-------------- src/storage/storage_driver.c | 32 +++++++++++++--------- src/uml/uml_driver.c | 20 +++++++------- src/xen/xen_driver.c | 5 ++-- 9 files changed, 119 insertions(+), 77 deletions(-) -- 2.5.5
Replace VIR_ERROR with virReportError and virReportSystemError --- src/uml/uml_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 923c3f6..fea3575 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,15 @@ 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: %s"), + uml_driver->monitorDir, + virStrerror(errno, ebuf, sizeof(ebuf))); goto error; } @@ -552,9 +552,9 @@ umlStateInitialize(bool privileged, 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: %s"), + uml_driver->monitorDir, + virStrerror(errno, ebuf, sizeof(ebuf))); goto error; } @@ -582,7 +582,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
On 05/23/2016 02:35 PM, Jovanka Gulicoska wrote:
Replace VIR_ERROR with virReportError and virReportSystemError --- src/uml/uml_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 923c3f6..fea3575 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,15 @@ 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: %s"), + uml_driver->monitorDir, + virStrerror(errno, ebuf, sizeof(ebuf))); goto error; }
I should have explained more about virReportSystemError :) It has some magic to automatically convert errno to strerror via virStrerror and append it to the message. So the manual invocation of virStrerror here is redundant and would give a double message like: Failed to create monitor directory $dir: $strerror: $strerror So all your virReportSystemError changes should be adjusted to remove the ebuf definition, and look like for example virReportSystemError(errno, _("Failed to create monitor directory %s"), uml_driver->monitorDir); This patch looks fine otherwise though Thanks, Cole
Replace VIR_ERROR with virReportSystemError --- src/xen/xen_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 3f5d80d..cc5c92d 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -532,8 +532,9 @@ 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': %s"), + priv->saveDir, + virStrerror(errno, ebuf, sizeof(ebuf))); goto error; } -- 2.5.5
Replace VIR_ERROR with virReportError --- src/bhyve/bhyve_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index c58286f..8afa599 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -88,8 +88,8 @@ bhyveAutostartDomain(virDomainObjPtr vm, void *opaque) ret = virBhyveProcessStart(data->conn, data->driver, vm, VIR_DOMAIN_RUNNING_BOOTED, 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()); } } virObjectUnlock(vm); -- 2.5.5
On 05/23/2016 02:35 PM, Jovanka Gulicoska wrote:
Replace VIR_ERROR with virReportError --- src/bhyve/bhyve_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index c58286f..8afa599 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -88,8 +88,8 @@ bhyveAutostartDomain(virDomainObjPtr vm, void *opaque) ret = virBhyveProcessStart(data->conn, data->driver, vm, VIR_DOMAIN_RUNNING_BOOTED, 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()); } } virObjectUnlock(vm);
We generally try to split lines over 80 columns if we can. So I tweaked this to virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart VM '%s': %s"), vm->def->name, virGetLastErrorMessage()); And pushed Thanks, Cole
Replace VIR_ERROR with virReportError --- src/libxl/libxl_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b8b4c24..e70e124 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -319,9 +319,9 @@ libxlAutostartDomain(virDomainObjPtr vm, if (vm->autostart && !virDomainObjIsActive(vm) && libxlDomainStartNew(driver, vm, false) < 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()); goto endjob; } -- 2.5.5
On 05/23/2016 02:35 PM, Jovanka Gulicoska wrote:
Replace VIR_ERROR with virReportError --- src/libxl/libxl_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b8b4c24..e70e124 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -319,9 +319,9 @@ libxlAutostartDomain(virDomainObjPtr vm,
if (vm->autostart && !virDomainObjIsActive(vm) && libxlDomainStartNew(driver, vm, false) < 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()); goto endjob; }
I split this long line similarly and pushed Thanks, Cole
Replace VIR_ERROR with virReportError and virReportSystemError --- src/node_device/node_device_hal.c | 24 ++++++++++++-------- src/node_device/node_device_udev.c | 45 ++++++++++++++++++++++++-------------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 6ddfad0..fd26692 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -641,24 +641,27 @@ 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 +686,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 +707,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 +758,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 6bff5ba..05253b6 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,7 @@ 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); } @@ -716,8 +718,8 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, filename = last_component(def->sysfs_path); if (!STRPREFIX(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; } @@ -874,8 +876,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; } @@ -1292,7 +1295,9 @@ 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; } @@ -1458,7 +1463,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; } @@ -1533,14 +1539,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; } @@ -1579,7 +1587,8 @@ 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'"), + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get udev device for syspath '%s' or '%s'"), DMI_DEVPATH, DMI_DEVPATH_FALLBACK); goto out; } @@ -1694,8 +1703,8 @@ static int nodeStateInitialize(bool privileged, * 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))); + virReportSystemError(errno, _("Failed to initialize libpciaccess: %s"), + virStrerror(pciret, ebuf, sizeof(ebuf))); ret = -1; goto out; } @@ -1717,7 +1726,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); ret = -1; @@ -1741,7 +1751,8 @@ static int nodeStateInitialize(bool privileged, priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { VIR_FREE(priv); - VIR_ERROR(_("udev_monitor_new_from_netlink returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_new_from_netlink returned NULL")); ret = -1; goto out_unlock; } -- 2.5.5
Replace VIR_ERROR with virReportError --- src/nwfilter/nwfilter_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2828b28..186830c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -215,8 +215,9 @@ nwfilterStateInitialize(bool privileged, */ if (sysbus && nwfilterDriverInstallDBusMatches(sysbus) < 0) { - VIR_ERROR(_("DBus matches could not be installed. Disabling nwfilter " - "driver")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("DBus matches could not be installed. " + "Disabling nwfilter driver")); /* * unfortunately this is fatal since virNWFilterTechDriversInit * may have caused the ebiptables driver to use the firewall tool -- 2.5.5
On 05/23/2016 02:35 PM, Jovanka Gulicoska wrote:
Replace VIR_ERROR with virReportError --- src/nwfilter/nwfilter_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2828b28..186830c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -215,8 +215,9 @@ nwfilterStateInitialize(bool privileged, */ if (sysbus && nwfilterDriverInstallDBusMatches(sysbus) < 0) { - VIR_ERROR(_("DBus matches could not be installed. Disabling nwfilter " - "driver")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("DBus matches could not be installed. " + "Disabling nwfilter driver")); /* * unfortunately this is fatal since virNWFilterTechDriversInit * may have caused the ebiptables driver to use the firewall tool
Thanks, I pushed this - Cole
Replace VIR_ERROR with virReportError and virReportSystemError --- src/qemu/qemu_driver.c | 55 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 249393a..e711204 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); @@ -481,8 +484,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 +499,11 @@ 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: %s"), + snapDir, vm->def->name, + virStrerror(errno, ebuf, sizeof(ebuf))); goto cleanup; } @@ -509,14 +516,17 @@ 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: %s"), + fullpath, + virStrerror(errno, ebuf, sizeof(ebuf))); VIR_FREE(fullpath); continue; } @@ -526,8 +536,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 +557,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
Replace VIR_ERROR with virReportError and virReportSystemError --- src/storage/storage_driver.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index eb5f688..879b3c7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -88,7 +88,8 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) goto error; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing backend %d"), pool->def->type); goto error; } @@ -98,8 +99,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) active = false; if (backend->checkPool && backend->checkPool(pool, &active) < 0) { - VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to initialize storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); goto error; } @@ -112,8 +114,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) if (backend->refreshPool(NULL, pool) < 0) { if (backend->stopPool) backend->stopPool(NULL, pool); - VIR_ERROR(_("Failed to restart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); goto error; } } @@ -172,8 +175,9 @@ storageDriverAutostart(void) !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(conn, pool) < 0) { - VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); virStoragePoolObjUnlock(pool); continue; } @@ -193,8 +197,9 @@ storageDriverAutostart(void) unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); - VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); } else { pool->active = true; } @@ -835,8 +840,10 @@ storagePoolUndefine(virStoragePoolPtr obj) errno != ENOENT && errno != ENOTDIR) { char ebuf[1024]; - VIR_ERROR(_("Failed to delete autostart link '%s': %s"), - pool->autostartLink, virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, + _("Failed to delete autostart link '%s': %s"), + pool->autostartLink, + virStrerror(errno, ebuf, sizeof(ebuf))); } VIR_FREE(pool->configFile); @@ -2301,7 +2308,8 @@ virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, if (virThreadCreate(&thread, false, virStorageVolPoolRefreshThread, opaque) < 0) { /* Not much else can be done */ - VIR_ERROR(_("Failed to create thread to handle pool refresh")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to create thread to handle pool refresh")); goto error; } return; /* Thread will free opaque data */ -- 2.5.5
On 05/23/2016 02:36 PM, Jovanka Gulicoska wrote:
Replace VIR_ERROR with virReportError and virReportSystemError --- src/storage/storage_driver.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index eb5f688..879b3c7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -88,7 +88,8 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) goto error;
if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing backend %d"), pool->def->type); goto error; }
@@ -98,8 +99,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) active = false; if (backend->checkPool && backend->checkPool(pool, &active) < 0) { - VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to initialize storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); goto error; }
@@ -112,8 +114,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) if (backend->refreshPool(NULL, pool) < 0) { if (backend->stopPool) backend->stopPool(NULL, pool); - VIR_ERROR(_("Failed to restart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); goto error; } } @@ -172,8 +175,9 @@ storageDriverAutostart(void) !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(conn, pool) < 0) { - VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); virStoragePoolObjUnlock(pool); continue; } @@ -193,8 +197,9 @@ storageDriverAutostart(void) unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); - VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), - pool->def->name, virGetLastErrorMessage()); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to autostart storage pool '%s': %s"), + pool->def->name, virGetLastErrorMessage()); } else { pool->active = true; }
The above changes look good, and I pushed this patch with these bottom two bits dropped.
@@ -835,8 +840,10 @@ storagePoolUndefine(virStoragePoolPtr obj) errno != ENOENT && errno != ENOTDIR) { char ebuf[1024]; - VIR_ERROR(_("Failed to delete autostart link '%s': %s"), - pool->autostartLink, virStrerror(errno, ebuf, sizeof(ebuf))); + virReportSystemError(errno, + _("Failed to delete autostart link '%s': %s"), + pool->autostartLink, + virStrerror(errno, ebuf, sizeof(ebuf))); }
This has the SystemError issue I outlined elsewhere, but actually this call site is a legitimate use of VIR_ERROR. storagePoolUndefine is called via direct user interaction, like virsh pool-undefine. In those cases, we only want to call virReport*Error if we hit a fatal error (cause the function to return an exit code). In this case, we want to log the failure, but not fail the API call over it, so VIR_ERROR is more appropriate IMO
VIR_FREE(pool->configFile); @@ -2301,7 +2308,8 @@ virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, if (virThreadCreate(&thread, false, virStorageVolPoolRefreshThread, opaque) < 0) { /* Not much else can be done */ - VIR_ERROR(_("Failed to create thread to handle pool refresh")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to create thread to handle pool refresh")); goto error; } return; /* Thread will free opaque data */
This is basically a similar case. Though this case will cause the function to fail, it's used as an async callback and not something via direct user interaction, so it's probably better to not mess with actual error reporting. It may make sense to convert to virReportError but would need closer inspection. Thanks, Cole
participants (2)
-
Cole Robinson -
Jovanka Gulicoska