Re: [PATCH] qemu: nicer error message if live disk snapshot unsupported

[Osier Yang <jyang@xxxxxxxxxx>]

On 2012年12月04日 05:33, Eric Blake wrote:
> Without this patch, attempts to create a disk snapshot when qemu
> is too old results in a cryptic message:
>
> virsh # snapshot-create 23 --disk-only
> error: operation failed: Failed to take snapshot: unknown command: 'snapshot_blkdev'
>
> Now it reports:
>
> virsh # snapshot-create 23 --disk-only
> error: unsupported configuration: live disk snapshot not supported with this QEMU binary
>
> All versions of qemu that support live disk snapshot also support
> QMP (basically upstream qemu 1.1 and later, and backport to RHEL 6.2).
>
> * src/qemu/qemu_capabilities.h (QEMU_CAPS_DISK_SNAPSHOT): New
> capability.
> * src/qemu/qemu_capabilities.c (qemuCaps): Track it.
> (qemuCapsProbeQMPCommands): Set it.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use
> it.
> * src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): Simplify.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot):
> Likewise.
> * src/qemu/qemu_monitor_text.h (qemuMonitorTextDiskSnapshot):
> Delete.
> * src/qemu/qemu_monitor_text.c (qemuMonitorTextDiskSnapshot):
> Likewise.

ACK'ed a bit earlier, there is no testing for this new flag,
and...

> ---
>   src/qemu/qemu_capabilities.c |    3 +++
>   src/qemu/qemu_capabilities.h |    1 +
>   src/qemu/qemu_driver.c       |    5 +++++
>   src/qemu/qemu_monitor.c      |   13 ++++---------
>   src/qemu/qemu_monitor_json.c |    6 ------
>   src/qemu/qemu_monitor_text.c |   37 -------------------------------------
>   src/qemu/qemu_monitor_text.h |    4 ----
>   7 files changed, 13 insertions(+), 56 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 6e34cdf..668935e 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -193,6 +193,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                 "drive-mirror", /* 115 */
>                 "usb-redir.bootindex",
>                 "usb-host.bootindex",
> +              "blockdev-snapshot-sync",
>       );
>
>   struct _qemuCaps {
> @@ -1948,6 +1949,8 @@ qemuCapsProbeQMPCommands(qemuCapsPtr caps,
>               qemuCapsSet(caps, QEMU_CAPS_VNC);
>           else if (STREQ(name, "drive-mirror"))
>               qemuCapsSet(caps, QEMU_CAPS_DRIVE_MIRROR);
> +        else if (STREQ(name, "blockdev-snapshot-sync"))
> +            qemuCapsSet(caps, QEMU_CAPS_DISK_SNAPSHOT);
>           VIR_FREE(name);
>       }
>       VIR_FREE(commands);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index f420c43..3da8672 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -155,6 +155,7 @@ enum qemuCapsFlags {
>       QEMU_CAPS_DRIVE_MIRROR       = 115, /* drive-mirror monitor command */
>       QEMU_CAPS_USB_REDIR_BOOTINDEX = 116, /* usb-redir.bootindex */
>       QEMU_CAPS_USB_HOST_BOOTINDEX = 117, /* usb-host.bootindex */
> +    QEMU_CAPS_DISK_SNAPSHOT      = 118, /* blockdev-snapshot-sync command */
>
>       QEMU_CAPS_LAST,                   /* this must always be the last item */
>   };
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8e838cd..fbacd8b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11230,6 +11230,11 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
>               virReportOOMError();
>               goto cleanup;
>           }
> +    } else if (!qemuCapsGet(priv->caps, QEMU_CAPS_DISK_SNAPSHOT)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("live disk snapshot not supported with this "
> +                         "QEMU binary"));
> +        goto cleanup;
>       }
>
>       /* No way to roll back if first disk succeeds but later disks
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index f85bb76..543f714 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2769,17 +2769,12 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>           return -1;
>       }
>
> -    if (mon->json) {
> +    if (mon->json)
>           ret = qemuMonitorJSONDiskSnapshot(mon, actions, device, file, format,
>                                             reuse);
> -    } else {
> -        if (actions || STRNEQ(format, "qcow2") || reuse) {
> -            virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                           _("text monitor lacks several snapshot features"));
> -            return -1;
> -        }
> -        ret = qemuMonitorTextDiskSnapshot(mon, device, file);
> -    }
> +    else
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("disk snapshot requires JSON monitor"));
>       return ret;

"ret" can be used without initialization.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list