[libvirt] [PATCH 0/3] Support domain snapshots with current QMP whithout savevm command
This is quite hacky since it involves falling back to HMP when savevm command is not found in QMP, which is something qemu monitor code was not designed to support. Hence, I'm providing 2 version of the first patch: 1.1/3 and 1.2/3. - 1.1/3 version only touches JSON monitor code but involves copy&paste of the snapshot code from text monitor - 1.2/3 touches more files but doesn't require duplicating the text monitor snapshot implementation into qemu_monitor_json.c. However, it results in somewhat funky call graphs: -> qemuMonitorJSONCreateSnapshot -> qemuMonitorTextCreateSnapshot -> qemuMonitorCommand (a macro expanding to qemuMonitorCommandWithFd) -> qemuMonitorJSONHumanCommandWithFd -> qemuMonitorJSONCommandWithFd The possibility to call qemuMonitorTextCreateSnapshot directly on JSON monitor is implemented by generalizing qemuMonitorCommandWithFd, which now either calls to qemuMonitorJSONHumanCommandWithFd or qemuMonitorTextCommandWithFd (former qemuMonitorCommandWithFd) depending on the monitor type. I prefer version 2 since it reuses text monitor implementation, but other may prefer version 1, which is a bit more local... Jiri Denemark (3): qemu: Fallback to HMP when savevm QMP command is not found qemu: Refactor qemuDomainSnapshotCreateXML qemu: Stop guest CPUs before creating a snapshot -- 1.7.4.1
qemu driver in libvirt gained support for creating domain snapshots almost a year ago in libvirt 0.8.0. Since then we enabled QMP support for qemu >= 0.13.0 but a QMP equivalent of savevm command is not implemented in current qemu (0.14.0) so the domain snapshot support is not very useful. This patch detects when the appropriate QMP command is not implemented and tries to use human-monitor-command (aka HMP passthrough) to run savevm HMP command. --- src/qemu/qemu_monitor_json.c | 144 +++++++++++++++++++++++++++++++++-------- 1 files changed, 116 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e6903a1..6dd7980 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -675,6 +675,65 @@ static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValueP } +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_FMT_PRINTF(3, 4) +qemuMonitorJSONHumanCommand(qemuMonitorPtr mon, + char **reply_str, + const char *fmt, ...) +{ + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr obj; + char *cmd_str = NULL; + va_list ap; + int ret = -1; + + va_start(ap, fmt); + if (virVasprintf(&cmd_str, fmt, ap) < 0) { + virReportOOMError(); + goto cleanup; + } + + cmd = qemuMonitorJSONMakeCommand("human-monitor-command", + "s:command-line", cmd_str, + NULL); + + if (!cmd || qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply)) + goto cleanup; + + if (!(obj = virJSONValueObjectGet(reply, "return"))) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("human monitor command was missing return data")); + goto cleanup; + } + + if (reply_str) { + const char *data; + + if ((data = virJSONValueGetString(obj))) + *reply_str = strdup(data); + else + *reply_str = strdup(""); + + if (!*reply_str) { + virReportOOMError(); + goto cleanup; + } + } + + ret = 0; + +cleanup: + va_end(ap); + VIR_FREE(cmd_str); + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONSetCapabilities(qemuMonitorPtr mon) { @@ -2389,6 +2448,47 @@ int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, return ret; } + +static int +qemuMonitorJSONCreateSnapshotHMP(qemuMonitorPtr mon, const char *name) +{ + char *reply = NULL; + int ret = -1; + + if (qemuMonitorJSONHumanCommand(mon, &reply, "savevm \"%s\"", name) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to take snapshot using HMP command")); + goto cleanup; + } + + if (strstr(reply, "Error while creating snapshot")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to take snapshot: %s"), reply); + goto cleanup; + } + else if (strstr(reply, "No block device can accept snapshots")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("this domain does not have a device to" + " take snapshots")); + goto cleanup; + } + else if (strstr(reply, "Could not open VM state file")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + goto cleanup; + } + else if (strstr(reply, "Error") + && strstr(reply, "while writing VM")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(reply); + return ret; +} + int qemuMonitorJSONCreateSnapshot(qemuMonitorPtr mon, const char *name) { int ret; @@ -2401,11 +2501,18 @@ int qemuMonitorJSONCreateSnapshot(qemuMonitorPtr mon, const char *name) if (!cmd) return -1; - ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0("savevm command not found, trying HMP"); + ret = qemuMonitorJSONCreateSnapshotHMP(mon, name); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply); +cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -2464,36 +2571,17 @@ int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; int ret = -1; - if (!hmp) { - cmd = virJSONValueFromString(cmd_str); + if (hmp) { + return qemuMonitorJSONHumanCommand(mon, reply_str, "%s", cmd_str); } else { - cmd = qemuMonitorJSONMakeCommand("human-monitor-command", - "s:command-line", cmd_str, - NULL); - } - - if (!cmd) - return -1; + if (!(cmd = virJSONValueFromString(cmd_str))) + goto cleanup; - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; - if (!hmp) { if (!(*reply_str = virJSONValueToString(reply))) goto cleanup; - } else if (qemuMonitorJSONCheckError(cmd, reply)) { - goto cleanup; - } else { - const char *data; - if (!(data = virJSONValueObjectGetString(reply, "return"))) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("human monitor command was missing return data")); - goto cleanup; - } - if (!(*reply_str = strdup(data))) { - virReportOOMError(); - goto cleanup; - } } ret = 0; -- 1.7.4.1
On Fri, Feb 25, 2011 at 07:04:08PM +0100, Jiri Denemark wrote:
qemu driver in libvirt gained support for creating domain snapshots almost a year ago in libvirt 0.8.0. Since then we enabled QMP support for qemu >= 0.13.0 but a QMP equivalent of savevm command is not implemented in current qemu (0.14.0) so the domain snapshot support is not very useful.
This patch detects when the appropriate QMP command is not implemented and tries to use human-monitor-command (aka HMP passthrough) to run savevm HMP command. --- src/qemu/qemu_monitor_json.c | 144 +++++++++++++++++++++++++++++++++-------- 1 files changed, 116 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e6903a1..6dd7980 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -675,6 +675,65 @@ static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValueP }
+static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_FMT_PRINTF(3, 4) +qemuMonitorJSONHumanCommand(qemuMonitorPtr mon, + char **reply_str, + const char *fmt, ...) +{ + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr obj; + char *cmd_str = NULL; + va_list ap; + int ret = -1; + + va_start(ap, fmt); + if (virVasprintf(&cmd_str, fmt, ap) < 0) { + virReportOOMError(); + goto cleanup; + }
This is a little dodgy because it doesn't take care of any data escaping of special characters in the args.
@@ -2389,6 +2448,47 @@ int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, return ret; }
+ +static int +qemuMonitorJSONCreateSnapshotHMP(qemuMonitorPtr mon, const char *name) +{ + char *reply = NULL; + int ret = -1; + + if (qemuMonitorJSONHumanCommand(mon, &reply, "savevm \"%s\"", name) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to take snapshot using HMP command")); + goto cleanup; + } + + if (strstr(reply, "Error while creating snapshot")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to take snapshot: %s"), reply); + goto cleanup; + } + else if (strstr(reply, "No block device can accept snapshots")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("this domain does not have a device to" + " take snapshots")); + goto cleanup; + } + else if (strstr(reply, "Could not open VM state file")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + goto cleanup; + } + else if (strstr(reply, "Error") + && strstr(reply, "while writing VM")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(reply); + return ret; +}
Could we re-factor the qemuMonitorTextCreateSnapshot() command so that the code which generates the command string & the code which checks the reply are separate callback methods. Then, this JSON code could do something like static int qemuMonitorJSONCreateSnapshotHMP(qemuMonitorPtr mon, const char *name) { if (!(cmdstr = qemuMonitorTextMakeCreateSnapshotCommand(...))) return -1; reply = qemuMonitorJSONHumanCommand(cmdstr) VIR_FREE(cmdstr); if (!reply) return -1; if (qemuMonitorTextCheckCreateSnapshotReply(reply) < 0) return -1; return 0; } Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
qemu driver in libvirt gained support for creating domain snapshots almost a year ago in libvirt 0.8.0. Since then we enabled QMP support for qemu >= 0.13.0 but a QMP equivalent of savevm command is not implemented in current qemu (0.14.0) so the domain snapshot support is not very useful. This patch detects when the appropriate QMP command is not implemented and tries to use human-monitor-command (aka HMP passthrough) to run savevm HMP command. --- src/qemu/qemu_monitor.c | 12 +++++ src/qemu/qemu_monitor.h | 8 +++- src/qemu/qemu_monitor_json.c | 95 +++++++++++++++++++++++++++++------------ src/qemu/qemu_monitor_json.h | 5 ++ src/qemu/qemu_monitor_text.c | 18 +++----- src/qemu/qemu_monitor_text.h | 5 ++ 6 files changed, 102 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dfb1aad..2d5124e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -742,6 +742,18 @@ cleanup: } +int qemuMonitorCommandWithFd(qemuMonitorPtr mon, + const char *cmd, + int scm_fd, + char **reply) +{ + if (mon->json) + return qemuMonitorJSONHumanCommandWithFd(mon, cmd, scm_fd, reply); + else + return qemuMonitorTextCommandWithFd(mon, cmd, scm_fd, reply); +} + + int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, virConnectPtr conn, const char *path, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0ea1330..7200db8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -133,9 +133,15 @@ void qemuMonitorUnlock(qemuMonitorPtr mon); int qemuMonitorRef(qemuMonitorPtr mon); int qemuMonitorUnref(qemuMonitorPtr mon); -/* This API is for use by the internal Text/JSON monitor impl code only */ +/* These APIs are for use by the internal Text/JSON monitor impl code only */ int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg); +int qemuMonitorCommandWithFd(qemuMonitorPtr mon, + const char *cmd, + int scm_fd, + char **reply); +# define qemuMonitorCommand(mon, cmd, reply) \ + qemuMonitorCommandWithFd(mon, cmd, -1, reply) /* XXX same comment about virConnectPtr as above */ int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e6903a1..afc29c8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -31,6 +31,7 @@ #include <string.h> #include <sys/time.h> +#include "qemu_monitor_text.h" #include "qemu_monitor_json.h" #include "qemu_command.h" #include "memory.h" @@ -676,6 +677,56 @@ static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValueP int +qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, + const char *cmd_str, + int scm_fd, + char **reply_str) +{ + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr obj; + int ret = -1; + + cmd = qemuMonitorJSONMakeCommand("human-monitor-command", + "s:command-line", cmd_str, + NULL); + + if (!cmd || qemuMonitorJSONCommandWithFd(mon, cmd, scm_fd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply)) + goto cleanup; + + if (!(obj = virJSONValueObjectGet(reply, "return"))) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("human monitor command was missing return data")); + goto cleanup; + } + + if (reply_str) { + const char *data; + + if ((data = virJSONValueGetString(obj))) + *reply_str = strdup(data); + else + *reply_str = strdup(""); + + if (!*reply_str) { + virReportOOMError(); + goto cleanup; + } + } + + ret = 0; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + +int qemuMonitorJSONSetCapabilities(qemuMonitorPtr mon) { int ret; @@ -2401,11 +2452,18 @@ int qemuMonitorJSONCreateSnapshot(qemuMonitorPtr mon, const char *name) if (!cmd) return -1; - ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0("savevm command not found, trying HMP"); + ret = qemuMonitorTextCreateSnapshot(mon, name); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply); +cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -2464,36 +2522,17 @@ int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; int ret = -1; - if (!hmp) { - cmd = virJSONValueFromString(cmd_str); + if (hmp) { + return qemuMonitorJSONHumanCommandWithFd(mon, cmd_str, -1, reply_str); } else { - cmd = qemuMonitorJSONMakeCommand("human-monitor-command", - "s:command-line", cmd_str, - NULL); - } - - if (!cmd) - return -1; + if (!(cmd = virJSONValueFromString(cmd_str))) + goto cleanup; - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; - if (!hmp) { if (!(*reply_str = virJSONValueToString(reply))) goto cleanup; - } else if (qemuMonitorJSONCheckError(cmd, reply)) { - goto cleanup; - } else { - const char *data; - if (!(data = virJSONValueObjectGetString(reply, "return"))) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("human monitor command was missing return data")); - goto cleanup; - } - if (!(*reply_str = strdup(data))) { - virReportOOMError(); - goto cleanup; - } } ret = 0; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4ae472a..3e0624d 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -34,6 +34,11 @@ int qemuMonitorJSONIOProcess(qemuMonitorPtr mon, size_t len, qemuMonitorMessagePtr msg); +int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, + const char *cmd, + int scm_fd, + char **reply); + int qemuMonitorJSONSetCapabilities(qemuMonitorPtr mon); int qemuMonitorJSONStartCPUs(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 0aed690..6949384 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -259,21 +259,15 @@ qemuMonitorCommandWithHandler(qemuMonitorPtr mon, return ret; } -static int -qemuMonitorCommandWithFd(qemuMonitorPtr mon, - const char *cmd, - int scm_fd, - char **reply) { +int +qemuMonitorTextCommandWithFd(qemuMonitorPtr mon, + const char *cmd, + int scm_fd, + char **reply) +{ return qemuMonitorCommandWithHandler(mon, cmd, NULL, NULL, scm_fd, reply); } -static int -qemuMonitorCommand(qemuMonitorPtr mon, - const char *cmd, - char **reply) { - return qemuMonitorCommandWithFd(mon, cmd, -1, reply); -} - static int qemuMonitorSendDiskPassphrase(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index b29dbcc..e6b27ec 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -35,6 +35,11 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon, size_t len, qemuMonitorMessagePtr msg); +int qemuMonitorTextCommandWithFd(qemuMonitorPtr mon, + const char *cmd, + int scm_fd, + char **reply); + int qemuMonitorTextStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); int qemuMonitorTextStopCPUs(qemuMonitorPtr mon); -- 1.7.4.1
--- src/qemu/qemu_driver.c | 125 +++++++++++++++++++++++++++++------------------ 1 files changed, 77 insertions(+), 48 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7fc08e8..ba046d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5987,6 +5987,81 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) return 1; } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; + int ret = -1; + int i; + + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) { + /* qemuFindQemuImgBinary set the error */ + goto cleanup; + } + + qemuimgarg[3] = snap->def->name; + + for (i = 0; i < vm->def->ndisks; i++) { + /* FIXME: we also need to handle LVM here */ + /* FIXME: if we fail halfway through this loop, we are in an + * inconsistent state. I'm not quite sure what to do about that + */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%s' does not support" + " snapshotting"), + vm->def->disks[i]->info.alias); + goto cleanup; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) { + virReportSystemError(errno, + _("Failed to run '%s' to create snapshot" + " '%s' from disk '%s'"), + qemuimgarg[0], snap->def->name, + vm->def->disks[i]->src); + goto cleanup; + } + } + } + + ret = 0; + +cleanup: + VIR_FREE(qemuimgarg[0]); + return ret; +} + +/* The domain is expected to be locked and active. */ +static int +qemuDomainSnapshotCreateActive(struct qemud_driver *driver, + virDomainObjPtr *vmptr, + virDomainSnapshotObjPtr snap) +{ + virDomainObjPtr vm = *vmptr; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + return -1; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (qemuDomainObjEndJob(vm) == 0) + *vmptr = NULL; + + return ret; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, unsigned int flags) @@ -5997,8 +6072,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def; - const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; - int i; virCheckFlags(0, NULL); @@ -6029,54 +6102,11 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, /* actually do the snapshot */ if (!virDomainObjIsActive(vm)) { - qemuimgarg[0] = qemuFindQemuImgBinary(); - if (qemuimgarg[0] == NULL) - /* qemuFindQemuImgBinary set the error */ + if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; - - qemuimgarg[3] = snap->def->name; - - for (i = 0; i < vm->def->ndisks; i++) { - /* FIXME: we also need to handle LVM here */ - /* FIXME: if we fail halfway through this loop, we are in an - * inconsistent state. I'm not quite sure what to do about that - */ - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("Disk device '%s' does not support snapshotting"), - vm->def->disks[i]->info.alias); - goto cleanup; - } - - qemuimgarg[4] = vm->def->disks[i]->src; - - if (virRun(qemuimgarg, NULL) < 0) { - virReportSystemError(errno, - _("Failed to run '%s' to create snapshot '%s' from disk '%s'"), - qemuimgarg[0], snap->def->name, - vm->def->disks[i]->src); - goto cleanup; - } - } - } } else { - qemuDomainObjPrivatePtr priv; - int ret; - - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) - goto cleanup; - priv = vm->privateData; - qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorCreateSnapshot(priv->mon, def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); - if (qemuDomainObjEndJob(vm) == 0) { - vm = NULL; - goto cleanup; - } - if (ret < 0) + if (qemuDomainSnapshotCreateActive(driver, &vm, snap) < 0) goto cleanup; } @@ -6106,7 +6136,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: - VIR_FREE(qemuimgarg[0]); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.7.4.1
On Fri, Feb 25, 2011 at 07:04:10PM +0100, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 125 +++++++++++++++++++++++++++++------------------ 1 files changed, 77 insertions(+), 48 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7fc08e8..ba046d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5987,6 +5987,81 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) return 1; }
+/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; + int ret = -1; + int i; + + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) { + /* qemuFindQemuImgBinary set the error */ + goto cleanup; + } + + qemuimgarg[3] = snap->def->name; + + for (i = 0; i < vm->def->ndisks; i++) { + /* FIXME: we also need to handle LVM here */ + /* FIXME: if we fail halfway through this loop, we are in an + * inconsistent state. I'm not quite sure what to do about that + */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%s' does not support" + " snapshotting"), + vm->def->disks[i]->info.alias); + goto cleanup; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) { + virReportSystemError(errno, + _("Failed to run '%s' to create snapshot" + " '%s' from disk '%s'"), + qemuimgarg[0], snap->def->name, + vm->def->disks[i]->src); + goto cleanup; + } + } + } + + ret = 0; + +cleanup: + VIR_FREE(qemuimgarg[0]); + return ret; +} + +/* The domain is expected to be locked and active. */ +static int +qemuDomainSnapshotCreateActive(struct qemud_driver *driver, + virDomainObjPtr *vmptr, + virDomainSnapshotObjPtr snap) +{ + virDomainObjPtr vm = *vmptr; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + return -1; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (qemuDomainObjEndJob(vm) == 0) + *vmptr = NULL; + + return ret; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, unsigned int flags) @@ -5997,8 +6072,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def; - const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; - int i;
virCheckFlags(0, NULL);
@@ -6029,54 +6102,11 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
/* actually do the snapshot */ if (!virDomainObjIsActive(vm)) { - qemuimgarg[0] = qemuFindQemuImgBinary(); - if (qemuimgarg[0] == NULL) - /* qemuFindQemuImgBinary set the error */ + if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; - - qemuimgarg[3] = snap->def->name; - - for (i = 0; i < vm->def->ndisks; i++) { - /* FIXME: we also need to handle LVM here */ - /* FIXME: if we fail halfway through this loop, we are in an - * inconsistent state. I'm not quite sure what to do about that - */ - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("Disk device '%s' does not support snapshotting"), - vm->def->disks[i]->info.alias); - goto cleanup; - } - - qemuimgarg[4] = vm->def->disks[i]->src; - - if (virRun(qemuimgarg, NULL) < 0) { - virReportSystemError(errno, - _("Failed to run '%s' to create snapshot '%s' from disk '%s'"), - qemuimgarg[0], snap->def->name, - vm->def->disks[i]->src); - goto cleanup; - } - } - } } else { - qemuDomainObjPrivatePtr priv; - int ret; - - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) - goto cleanup; - priv = vm->privateData; - qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorCreateSnapshot(priv->mon, def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); - if (qemuDomainObjEndJob(vm) == 0) { - vm = NULL; - goto cleanup; - } - if (ret < 0) + if (qemuDomainSnapshotCreateActive(driver, &vm, snap) < 0) goto cleanup; }
@@ -6106,7 +6136,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name);
cleanup: - VIR_FREE(qemuimgarg[0]); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver);
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Mar 03, 2011 at 14:58:49 +0000, Daniel P. Berrange wrote:
On Fri, Feb 25, 2011 at 07:04:10PM +0100, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 125 +++++++++++++++++++++++++++++------------------ 1 files changed, 77 insertions(+), 48 deletions(-)
ACK
Thanks, pushed. Jirka
--- src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++++++----- 1 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba046d7..a02d1b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,21 +6041,47 @@ cleanup: /* The domain is expected to be locked and active. */ static int -qemuDomainSnapshotCreateActive(struct qemud_driver *driver, +qemuDomainSnapshotCreateActive(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr *vmptr, virDomainSnapshotObjPtr snap) { virDomainObjPtr vm = *vmptr; qemuDomainObjPrivatePtr priv = vm->privateData; - int ret; + bool resume = false; + int ret = -1; if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) return -1; + if (vm->state == VIR_DOMAIN_RUNNING) { + /* savevm monitor command pauses the domain emitting an event which + * confuses libvirt since it's not notified when qemu resumes the + * domain. Thus we stop and start CPUs ourselves. + */ + if (qemuProcessStopCPUs(driver, vm) < 0) + goto cleanup; + + resume = true; + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitorWithDriver(driver, vm); +cleanup: + if (resume && virDomainObjIsActive(vm) && + qemuProcessStartCPUs(driver, vm, conn) < 0 && + virGetLastError() == NULL) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("resuming after snapshot failed")); + } + if (qemuDomainObjEndJob(vm) == 0) *vmptr = NULL; @@ -6100,18 +6126,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; + snap->def->state = vm->state; + /* actually do the snapshot */ if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; } else { - if (qemuDomainSnapshotCreateActive(driver, &vm, snap) < 0) + if (qemuDomainSnapshotCreateActive(domain->conn, driver, + &vm, snap) < 0) goto cleanup; } - snap->def->state = vm->state; - /* FIXME: if we fail after this point, there's not a whole lot we can * do; we've successfully taken the snapshot, and we are now running * on it, so we have to go forward the best we can -- 1.7.4.1
On Fri, Feb 25, 2011 at 07:04:11PM +0100, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++++++----- 1 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba046d7..a02d1b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,21 +6041,47 @@ cleanup:
/* The domain is expected to be locked and active. */ static int -qemuDomainSnapshotCreateActive(struct qemud_driver *driver, +qemuDomainSnapshotCreateActive(virConnectPtr conn, + struct qemud_driver *driver, virDomainObjPtr *vmptr, virDomainSnapshotObjPtr snap) { virDomainObjPtr vm = *vmptr; qemuDomainObjPrivatePtr priv = vm->privateData; - int ret; + bool resume = false; + int ret = -1;
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) return -1;
+ if (vm->state == VIR_DOMAIN_RUNNING) { + /* savevm monitor command pauses the domain emitting an event which + * confuses libvirt since it's not notified when qemu resumes the + * domain. Thus we stop and start CPUs ourselves. + */
Not having a resume event from QEMU sounds like a QEMU bug, right ?
+ if (qemuProcessStopCPUs(driver, vm) < 0) + goto cleanup; + + resume = true; + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitorWithDriver(driver, vm);
+cleanup: + if (resume && virDomainObjIsActive(vm) && + qemuProcessStartCPUs(driver, vm, conn) < 0 && + virGetLastError() == NULL) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("resuming after snapshot failed")); + } + if (qemuDomainObjEndJob(vm) == 0) *vmptr = NULL;
@@ -6100,18 +6126,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup;
+ snap->def->state = vm->state; + /* actually do the snapshot */ if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; } else { - if (qemuDomainSnapshotCreateActive(driver, &vm, snap) < 0) + if (qemuDomainSnapshotCreateActive(domain->conn, driver, + &vm, snap) < 0) goto cleanup; }
- snap->def->state = vm->state; - /* FIXME: if we fail after this point, there's not a whole lot we can * do; we've successfully taken the snapshot, and we are now running * on it, so we have to go forward the best we can
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Mar 03, 2011 at 14:58:22 +0000, Daniel P. Berrange wrote:
On Fri, Feb 25, 2011 at 07:04:11PM +0100, Jiri Denemark wrote:
--- src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++++++----- 1 files changed, 32 insertions(+), 5 deletions(-)
ACK
Daniel
Thanks, pushed. Jirka
On Fri, Feb 25, 2011 at 07:04:07PM +0100, Jiri Denemark wrote:
This is quite hacky since it involves falling back to HMP when savevm command is not found in QMP, which is something qemu monitor code was not designed to support. Hence, I'm providing 2 version of the first patch: 1.1/3 and 1.2/3.
- 1.1/3 version only touches JSON monitor code but involves copy&paste of the snapshot code from text monitor
- 1.2/3 touches more files but doesn't require duplicating the text monitor snapshot implementation into qemu_monitor_json.c. However, it results in somewhat funky call graphs:
-> qemuMonitorJSONCreateSnapshot -> qemuMonitorTextCreateSnapshot -> qemuMonitorCommand (a macro expanding to qemuMonitorCommandWithFd) -> qemuMonitorJSONHumanCommandWithFd -> qemuMonitorJSONCommandWithFd
The possibility to call qemuMonitorTextCreateSnapshot directly on JSON monitor is implemented by generalizing qemuMonitorCommandWithFd, which now either calls to qemuMonitorJSONHumanCommandWithFd or qemuMonitorTextCommandWithFd (former qemuMonitorCommandWithFd) depending on the monitor type.
This call path rather makes my head hurt but the lack of duplicated code is attractive.
I prefer version 2 since it reuses text monitor implementation, but other may prefer version 1, which is a bit more local...
I've suggested a possible 3rd alternative in my reply to v1 Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Mar 03, 2011 at 03:04:58PM +0000, Daniel P. Berrange wrote:
On Fri, Feb 25, 2011 at 07:04:07PM +0100, Jiri Denemark wrote:
This is quite hacky since it involves falling back to HMP when savevm command is not found in QMP, which is something qemu monitor code was not designed to support. Hence, I'm providing 2 version of the first patch: 1.1/3 and 1.2/3.
- 1.1/3 version only touches JSON monitor code but involves copy&paste of the snapshot code from text monitor
- 1.2/3 touches more files but doesn't require duplicating the text monitor snapshot implementation into qemu_monitor_json.c. However, it results in somewhat funky call graphs:
-> qemuMonitorJSONCreateSnapshot -> qemuMonitorTextCreateSnapshot -> qemuMonitorCommand (a macro expanding to qemuMonitorCommandWithFd)
Perhaps we could just add 'HMP' in the name here to indicate this is just used for HMP based commands, and not the pure JSON ones.
-> qemuMonitorJSONHumanCommandWithFd -> qemuMonitorJSONCommandWithFd
The possibility to call qemuMonitorTextCreateSnapshot directly on JSON monitor is implemented by generalizing qemuMonitorCommandWithFd, which now either calls to qemuMonitorJSONHumanCommandWithFd or qemuMonitorTextCommandWithFd (former qemuMonitorCommandWithFd) depending on the monitor type.
This call path rather makes my head hurt but the lack of duplicated code is attractive.
I prefer version 2 since it reuses text monitor implementation, but other may prefer version 1, which is a bit more local...
I've suggested a possible 3rd alternative in my reply to v1
Ok, so having looked again at all 3 possible impls now, I'm going to change my mind and agree with you that 1.2/3 is the likely the best option due to the simplicity of adding HMP fallback to arbitrary commands. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Jiri Denemark