[libvirt] [PATCH] qemu: improve errors related to offline domains

https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out that attempting 'virsh blockpull' on an offline domain gave a misleading error message about qemu lacking support for the operation, even when qemu was specifically updated to support it. The real problem is that we have several capabilities that are only determined when starting a domain, and therefore are still clear when first working with an inactive domain (namely, any capability set by qemuMonitorJSONCheckCommands). While this patch was able to hoist an existing check in one of the three culprits, it had to add redundant checks in the other two places (because you always have to check for an active domain after obtaining a VM job lock, but the capability bits were being checked prior to obtaining the job lock). Someday it would be nice to patch libvirt to cache the set of capabilities per qemu binary (as determined by inode and timestamp), rather than re-probing the binary every time a domain is started, and to teach the cache how to query the monitor during the one time the probe is made rather than having to wait until a guest is started; then, a capability probe would succeed even for offline guests because it just refers to the cache, and the single check for an active domain after grabbing the job lock would be sufficient. But since that will involve a lot more coding, I'm happy to go with this simpler solution for an immediate solution. * src/qemu/qemu_driver.c (qemuDomainPMSuspendForDuration) (qemuDomainSnapshotCreateXML, qemuDomainBlockJobImpl): Check for offline state before checking an online-only cap. --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++------ 1 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3aa34d..3a1c1c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10389,6 +10389,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots of inactive domains not " + "implemented yet")); + goto cleanup; + } if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, false) < 0) @@ -10443,12 +10449,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, * makes sense, such as checking that qemu-img recognizes the * snapshot name in at least one of the domain's disks? */ } else if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots of inactive domains not " - "implemented yet")); - goto cleanup; - } if (qemuDomainSnapshotCreateDiskActive(domain->conn, driver, &vm, snap, flags) < 0) goto cleanup; @@ -11642,6 +11642,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + priv = vm->privateData; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { async = true; @@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, priv = vm->privateData; + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_WAKEUP) && (target == VIR_NODE_SUSPEND_TARGET_MEM || target == VIR_NODE_SUSPEND_TARGET_HYBRID)) { -- 1.7.7.6

On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out that attempting 'virsh blockpull' on an offline domain gave a misleading error message about qemu lacking support for the operation, even when qemu was specifically updated to support it. The real problem is that we have several capabilities that are only determined when starting a domain, and therefore are still clear when first working with an inactive domain (namely, any capability set by qemuMonitorJSONCheckCommands). [...]
* src/qemu/qemu_driver.c (qemuDomainPMSuspendForDuration) (qemuDomainSnapshotCreateXML, qemuDomainBlockJobImpl): Check for offline state before checking an online-only cap. --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++------ 1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3aa34d..3a1c1c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10389,6 +10389,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup;
if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots of inactive domains not " + "implemented yet")); + goto cleanup; + } if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, false)< 0) @@ -10443,12 +10449,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, * makes sense, such as checking that qemu-img recognizes the * snapshot name in at least one of the domain's disks? */ } else if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots of inactive domains not " - "implemented yet")); - goto cleanup; - } if (qemuDomainSnapshotCreateDiskActive(domain->conn, driver, &vm, snap, flags)< 0) goto cleanup;
So this moves the check to an earlier place.
@@ -11642,6 +11642,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + priv = vm->privateData; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { async = true;
'goto cleanup' here -- same comment as below.
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm->privateData;
+ if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + Nit, you could move this above the priv =. Neither endjob nor cleanup need this priv. But 'goto endjob' is wrong (replace with 'goto cleanup'), since only later (~line 12655) the
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; is run. Stefan

On 04/26/2012 02:11 PM, Stefan Berger wrote:
On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out that attempting 'virsh blockpull' on an offline domain gave a misleading error message about qemu lacking support for the operation, even when qemu was specifically updated to support it. The real problem is that we have several capabilities that are only determined when starting a domain, and therefore are still clear when first working with an inactive domain (namely, any capability set by qemuMonitorJSONCheckCommands).
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm->privateData;
+ if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + Nit, you could move this above the priv =. Neither endjob nor cleanup need this priv. But 'goto endjob' is wrong (replace with 'goto cleanup'),
D'oh - too much copy-and-paste! You're obviously right (and this is why we do code review). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/26/2012 02:17 PM, Eric Blake wrote:
On 04/26/2012 02:11 PM, Stefan Berger wrote:
On 04/26/2012 03:50 PM, Eric Blake wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=816662 pointed out that attempting 'virsh blockpull' on an offline domain gave a misleading error message about qemu lacking support for the operation, even when qemu was specifically updated to support it. The real problem is that we have several capabilities that are only determined when starting a domain, and therefore are still clear when first working with an inactive domain (namely, any capability set by qemuMonitorJSONCheckCommands).
@@ -12637,6 +12643,12 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
priv = vm->privateData;
+ if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + Nit, you could move this above the priv =. Neither endjob nor cleanup need this priv. But 'goto endjob' is wrong (replace with 'goto cleanup'),
D'oh - too much copy-and-paste! You're obviously right (and this is why we do code review).
Pushed with the two labels fixed, per your ACK on IRC. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Stefan Berger