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

[Stefan Berger]

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