[libvirt] [PATCH] qemu: nicer error message if live disk snapshot unsupported
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. --- 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; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9b6702a..0cd66b6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2986,12 +2986,6 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) goto cleanup; - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - VIR_DEBUG("blockdev-snapshot-sync command not found, trying HMP"); - ret = qemuMonitorTextDiskSnapshot(mon, device, file); - goto cleanup; - } - ret = qemuMonitorJSONCheckError(cmd, reply); } diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 43e5449..fa10600 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2958,43 +2958,6 @@ cleanup: return ret; } -int -qemuMonitorTextDiskSnapshot(qemuMonitorPtr mon, const char *device, - const char *file) -{ - char *cmd = NULL; - char *reply = NULL; - int ret = -1; - char *safename; - - if (!(safename = qemuMonitorEscapeArg(file)) || - virAsprintf(&cmd, "snapshot_blkdev %s \"%s\"", device, safename) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (qemuMonitorHMPCommand(mon, cmd, &reply)) - goto cleanup; - - if (strstr(reply, "error while creating qcow2") != NULL || - strstr(reply, "unknown command:") != NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to take snapshot: %s"), reply); - goto cleanup; - } - - /* XXX Should we scrape 'info block' output for - * 'device:... file=name backing_file=oldname' to make sure the - * command succeeded? */ - - ret = 0; - -cleanup: - VIR_FREE(safename); - VIR_FREE(cmd); - VIR_FREE(reply); - return ret; -} int qemuMonitorTextArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply) diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 079fbdc..97abad6 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -217,10 +217,6 @@ int qemuMonitorTextCreateSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorTextDeleteSnapshot(qemuMonitorPtr mon, const char *name); -int qemuMonitorTextDiskSnapshot(qemuMonitorPtr mon, - const char *device, - const char *file); - int qemuMonitorTextArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply); -- 1.7.1
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. --- 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",
Shouldn't this be VIR_ERR_OPERATION_INVALID instead? As it's not related with config.
+ _("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",
Likewise. ACK otherwise. Osier
On Tue, Dec 04, 2012 at 23:10:18 +0800, Osier Yang wrote:
On 2012年12月04日 05:33, Eric Blake wrote: ...
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",
Shouldn't this be VIR_ERR_OPERATION_INVALID instead? As it's not related with config.
The right code is actually VIR_ERR_OPERATION_UNSUPPORTED. Jirka
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.
On 12/04/2012 08:43 AM, Osier Yang wrote:
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
* src/qemu/qemu_capabilities.h (QEMU_CAPS_DISK_SNAPSHOT): New capability.
ACK'ed a bit earlier, there is no testing for this new flag,
Alas, we really don't have any tests already in place for capabilities that are set only by probing the existence of a QMP command. However, Dan has done enough framework that we can fake a QMP sequence, so maybe it is worth expanding that concept to be able to fake the list of QMP commands supported by various qemu releases. But I think it's big enough to save for another patch.
and...
"ret" can be used without initialization.
Good catch.
} + } else if (!qemuCapsGet(priv->caps, QEMU_CAPS_DISK_SNAPSHOT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
Shouldn't this be VIR_ERR_OPERATION_INVALID instead? As it's not related with config.
The right code is actually VIR_ERR_OPERATION_UNSUPPORTED.
This was from copy-and-paste from elsewhere in qemu_driver.c, but indeed OPERATION_UNSUPPORTED sounds best. I've fixed those problems, and will push shortly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Osier Yang