[libvirt] [PATCH 0/3] qemu: hotplig: Improve error reporting
Add helpers to remember existing errors when they might be overwritten and use it to fix "unknown error" reported when attaching disk with lock manager enabled. Peter Krempa (3): util: error: Add helpers for saving and restoring of last error qemu: hotplug: Use new helpers for storing libvirt errors qemu: Restore errors when rolling back disk image state src/libvirt_private.syms | 2 + src/qemu/qemu_hotplug.c | 97 ++++++++++++++++-------------------------------- src/util/virerror.c | 45 ++++++++++++++++++++++ src/util/virerror.h | 3 ++ 4 files changed, 82 insertions(+), 65 deletions(-) -- 2.14.1
Some cleanup paths overwrite a usefull error message with a less useful one and we then try to preserve the original message. The handlers added in this patch will simplify the operations since they are designed right for the purpose. --- src/libvirt_private.syms | 2 ++ src/util/virerror.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virerror.h | 3 +++ 3 files changed, 50 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f30a04b14..62e05186d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1604,6 +1604,8 @@ ebtablesRemoveForwardAllowIn; virDispatchError; virErrorCopyNew; virErrorInitialize; +virErrorPreserveLast; +virErrorRestore; virErrorSetErrnoFromLastError; virLastErrorIsSystemErrno; virRaiseErrorFull; diff --git a/src/util/virerror.c b/src/util/virerror.c index a5a2d6ed1..1f15c5dbb 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -371,6 +371,51 @@ virSaveLastError(void) return to; } + +/** + * virErrorPreserveLast: + * @saveerr: pointer to virErrorPtr for storing last error object + * + * Preserves the currently set last error (for the thread) into @saveerr so that + * it can be restored via virErrorRestore(). @saveerr must be passed to + * virErrorRestore() + */ +void +virErrorPreserveLast(virErrorPtr *saveerr) +{ + int saved_errno = errno; + virErrorPtr lasterr = virGetLastError(); + + *saveerr = NULL; + + if (lasterr) + *saveerr = virErrorCopyNew(lasterr); + + errno = saved_errno; +} + + +/** + * virErrorRestore: + * @savederr: error object holding saved error + * + * Restores the error passed via @savederr and clears associated memory. + */ +void +virErrorRestore(virErrorPtr *savederr) +{ + int saved_errno = errno; + + if (!*savederr) + return; + + virSetError(*savederr); + virFreeError(*savederr); + *savederr = NULL; + errno = saved_errno; +} + + /** * virResetError: * @err: pointer to the virError to clean up diff --git a/src/util/virerror.h b/src/util/virerror.h index 234864812..54530d081 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -196,4 +196,7 @@ void virErrorSetErrnoFromLastError(void); bool virLastErrorIsSystemErrno(int errnum); +void virErrorPreserveLast(virErrorPtr *saveerr); +void virErrorRestore(virErrorPtr *savederr); + #endif -- 2.14.1
The helpers allow to simplify restoring original errors in most cases. --- src/qemu/qemu_hotplug.c | 93 +++++++++++++++---------------------------------- 1 file changed, 28 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b365078ec..35d73f74e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -433,7 +433,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); @@ -444,11 +444,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } - + virErrorRestore(&orig_err); virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -722,7 +718,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); @@ -732,10 +728,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (encobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -819,16 +812,13 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -1356,7 +1346,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (!virDomainObjIsActive(vm)) goto cleanup; - originalError = virSaveLastError(); + virErrorPreserveLast(&originalError); if (vlan < 0) { if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { char *netdev_name; @@ -1387,8 +1377,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, VIR_FREE(hostnet_name); } } - virSetError(originalError); - virFreeError(originalError); + virErrorRestore(&originalError); goto cleanup; } @@ -1562,7 +1551,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, if (!tlsAlias && !secAlias) return; - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; @@ -1576,10 +1565,7 @@ qemuDomainDelTLSObjects(virQEMUDriverPtr driver, ignore_value(qemuDomainObjExitMonitor(driver, vm)); cleanup: - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); } @@ -1621,12 +1607,9 @@ qemuDomainAddTLSObjects(virQEMUDriverPtr driver, return qemuDomainObjExitMonitor(driver, vm); error: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); qemuDomainDelTLSObjects(driver, vm, asyncJob, secAlias, tlsAlias); return -1; @@ -1788,15 +1771,12 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); /* detach associated chardev on error */ if (chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, secAlias, tlsAlias); goto audit; @@ -2051,15 +2031,12 @@ int qemuDomainAttachChrDevice(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); /* detach associated chardev on error */ if (chardevAttached) qemuMonitorDetachCharDev(priv->mon, charAlias); ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, secAlias, tlsAlias); @@ -2202,17 +2179,14 @@ qemuDomainAttachRNGDevice(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (objAdded) ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && chardevAdded) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); qemuDomainDelTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE, secAlias, tlsAlias); @@ -2349,15 +2323,12 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (objAdded) ignore_value(qemuMonitorDelObject(priv->mon, objalias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) mem = NULL; - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); if (!mem) goto audit; @@ -2368,10 +2339,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, mem = NULL; /* reset the mlock limit */ - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); ignore_value(qemuDomainAdjustMaxMemLock(vm)); - virSetError(orig_err); - virFreeError(orig_err); + virErrorRestore(&orig_err); goto audit; } @@ -2561,17 +2531,14 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drvstr, devstr); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); virDomainAuditHostdev(vm, hostdev, "attach", false); @@ -2846,7 +2813,7 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, return ret; exit_monitor: - orig_err = virSaveLastError(); + virErrorPreserveLast(&orig_err); if (release_backing) { if (shmem->server.enabled) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); @@ -2857,10 +2824,7 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) release_address = false; - if (orig_err) { - virSetError(orig_err); - virFreeError(orig_err); - } + virErrorRestore(&orig_err); goto audit; } @@ -2948,10 +2912,9 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm, _("failed to add new filter rules to '%s' " "- attempting to restore old rules"), olddev->ifname); - errobj = virSaveLastError(); + virErrorPreserveLast(&errobj); ignore_value(virDomainConfNWFilterInstantiate(vm->def->uuid, olddev)); - virSetError(errobj); - virFreeError(errobj); + virErrorRestore(&errobj); return -1; } return 0; -- 2.14.1
Some operations done to rollback disk image labelling and locking might overwrite (or clear) the actual error. Remember the original error when tearing down disk access so that it's not obscured. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1461301 --- src/qemu/qemu_hotplug.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 35d73f74e..7dd6e5fd9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -94,6 +94,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; virStorageSourcePtr origsrc = NULL; + virErrorPtr orig_err = NULL; if (overridesrc) { origsrc = disk->src; @@ -102,6 +103,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, /* just tear down the disk access */ if (teardown) { + virErrorPreserveLast(&orig_err); ret = 0; goto rollback_cgroup; } @@ -145,6 +147,8 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver, if (origsrc) disk->src = origsrc; + virErrorRestore(&orig_err); + virObjectUnref(cfg); return ret; -- 2.14.1
On Tue, Sep 12, 2017 at 12:09:43PM +0200, Peter Krempa wrote:
Add helpers to remember existing errors when they might be overwritten and use it to fix "unknown error" reported when attaching disk with lock manager enabled.
Peter Krempa (3): util: error: Add helpers for saving and restoring of last error qemu: hotplug: Use new helpers for storing libvirt errors qemu: Restore errors when rolling back disk image state
src/libvirt_private.syms | 2 + src/qemu/qemu_hotplug.c | 97 ++++++++++++++++-------------------------------- src/util/virerror.c | 45 ++++++++++++++++++++++ src/util/virerror.h | 3 ++ 4 files changed, 82 insertions(+), 65 deletions(-)
ACK serues Jan
participants (2)
-
Ján Tomko -
Peter Krempa