When reverting a running domain to a paused snapshot, the event that fires should mention that the domain is suspended. * include/libvirt/libvirt.h.in (VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT): New sub-event. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- include/libvirt/libvirt.h.in | 3 ++- src/qemu/qemu_driver.c | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa29fb6..a625479 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2010,7 +2010,7 @@ typedef enum { VIR_DOMAIN_EVENT_STARTED_BOOTED = 0, /* Normal startup from boot */ VIR_DOMAIN_EVENT_STARTED_MIGRATED = 1, /* Incoming migration from another host */ VIR_DOMAIN_EVENT_STARTED_RESTORED = 2, /* Restored from a state file */ - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from snapshot */ + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from running snapshot */ } virDomainEventStartedDetailType; /** @@ -2023,6 +2023,7 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1, /* Suspended for offline migration */ VIR_DOMAIN_EVENT_SUSPENDED_IOERROR = 2, /* Suspended due to a disk I/O error */ VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG = 3, /* Suspended due to a watchdog firing */ + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 4, /* Restored from paused snapshot */ } virDomainEventSuspendedDetailType; /** diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57ad3d1..12f8179 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8808,14 +8808,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); } else { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } - - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, -- 1.7.4.4

There are two classes of management apps that track events - one that only cares about on/off (and only needs to track EVENT_STARTED and EVENT_STOPPED), and one that cares about paused/running (also tracks EVENT_SUSPENDED/EVENT_RESUMED). To keep both classes happy, any transition that can go from inactive to paused must emit two back-to-back events - one for started and one for suspended (since later resuming of the domain will only send RESUMED, but the first class isn't tracking that). This also fixes a bug where virDomainCreateWithFlags with the VIR_DOMAIN_START_PAUSED flag failed to start paused when restoring from a managed save image. * include/libvirt/libvirt.h.in (VIR_DOMAIN_EVENT_SUSPENDED_RESTORED) (VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT) (VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT): New sub-events. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use them. (qemuDomainSaveImageStartVM): Likewise, and add parameter. (qemudDomainCreate, qemuDomainObjStart): Send suspended event when starting paused. (qemuDomainObjRestore): Add parameter. (qemuDomainObjStart, qemuDomainRestoreFlags): Update callers. * examples/domain-events/events-c/event-test.c (eventDetailToString): Map new detail strings. --- I think this addresses Dan's findings about events related to state transitions. It turned a 13-line v3 into a 109-line patch. I'll repost my entire snapshot series as v4 once I finish rebasing on top of this (since I know it introduces some merge conflicts with later patches in the series). examples/domain-events/events-c/event-test.c | 37 +++++++++++++-- include/libvirt/libvirt.h.in | 6 ++- src/qemu/qemu_driver.c | 66 +++++++++++++++++++++----- 3 files changed, 90 insertions(+), 19 deletions(-) diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 4766a0d..6a3ed26 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -87,19 +87,45 @@ static const char *eventDetailToString(int event, int detail) { case VIR_DOMAIN_EVENT_STARTED_RESTORED: ret = "Restored"; break; + case VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; } break; case VIR_DOMAIN_EVENT_SUSPENDED: - if (detail == VIR_DOMAIN_EVENT_SUSPENDED_PAUSED) + switch (detail) { + case VIR_DOMAIN_EVENT_SUSPENDED_PAUSED: ret = "Paused"; - else if (detail == VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED) + break; + case VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED: ret = "Migrated"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_IOERROR: + ret = "I/O Error"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG: + ret = "Watchdog"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_RESTORED: + ret = "Restored"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; + } break; case VIR_DOMAIN_EVENT_RESUMED: - if (detail == VIR_DOMAIN_EVENT_RESUMED_UNPAUSED) + switch (detail) { + case VIR_DOMAIN_EVENT_RESUMED_UNPAUSED: ret = "Unpaused"; - else if (detail == VIR_DOMAIN_EVENT_RESUMED_MIGRATED) + break; + case VIR_DOMAIN_EVENT_RESUMED_MIGRATED: ret = "Migrated"; + break; + case VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; + } break; case VIR_DOMAIN_EVENT_STOPPED: switch (detail) { @@ -121,6 +147,9 @@ static const char *eventDetailToString(int event, int detail) { case VIR_DOMAIN_EVENT_STOPPED_FAILED: ret = "Failed"; break; + case VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; } break; } diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 53a2f7d..edd197c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -116,8 +116,7 @@ typedef enum { VIR_DOMAIN_PAUSED_DUMP = 4, /* paused for offline core dump */ VIR_DOMAIN_PAUSED_IOERROR = 5, /* paused due to a disk I/O error */ VIR_DOMAIN_PAUSED_WATCHDOG = 6, /* paused due to a watchdog event */ - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* restored from a snapshot which was - * taken while domain was paused */ + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* paused after restoring from snapshot */ } virDomainPausedReason; typedef enum { @@ -2024,6 +2023,8 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1, /* Suspended for offline migration */ VIR_DOMAIN_EVENT_SUSPENDED_IOERROR = 2, /* Suspended due to a disk I/O error */ VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG = 3, /* Suspended due to a watchdog firing */ + VIR_DOMAIN_EVENT_SUSPENDED_RESTORED = 4, /* Restored from paused state file */ + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 5, /* Restored from paused snapshot */ } virDomainEventSuspendedDetailType; /** @@ -2034,6 +2035,7 @@ typedef enum { typedef enum { VIR_DOMAIN_EVENT_RESUMED_UNPAUSED = 0, /* Normal resume due to admin unpause */ VIR_DOMAIN_EVENT_RESUMED_MIGRATED = 1, /* Resumed for completion of migration */ + VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT = 2, /* Resumed from snapshot */ } virDomainEventResumedDetailType; /** diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 737eec8..6becd31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1268,6 +1268,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; + virDomainEventPtr event2 = NULL; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY, NULL); @@ -1315,6 +1316,16 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_BOOTED); + if (event && (flags & VIR_DOMAIN_START_PAUSED)) { + /* There are two classes of event-watching clients - those + * that only care about on/off (and must see a started event + * no matter what, but don't care about suspend events), and + * those that also care about running/paused. To satisfy both + * client types, we have to send two events. */ + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + } virDomainAuditStart(vm, "booted", true); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); @@ -1328,8 +1339,11 @@ cleanup: virDomainDefFree(def); if (vm) virDomainObjUnlock(vm); - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); + } qemuDriverUnlock(driver); return dom; } @@ -3890,7 +3904,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, virDomainObjPtr vm, int *fd, const struct qemud_save_header *header, - const char *path) + const char *path, + bool start_paused) { int ret = -1; virDomainEventPtr event; @@ -3961,8 +3976,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainEventQueue(driver, event); - /* If it was running before, resume it now. */ - if (header->was_running) { + /* If it was running before, resume it now unless caller requested pause. */ + if (header->was_running && !start_paused) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_RESTORED, QEMU_ASYNC_JOB_NONE) < 0) { @@ -3975,6 +3990,14 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, VIR_WARN("Failed to save status on vm %s", vm->def->name); goto out; } + } else { + int detail = (start_paused ? VIR_DOMAIN_EVENT_SUSPENDED_PAUSED : + VIR_DOMAIN_EVENT_SUSPENDED_RESTORED); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + if (event) + qemuDomainEventQueue(driver, event); } ret = 0; @@ -4026,7 +4049,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, + false); if (virFileDirectFdClose(directFd) < 0) VIR_WARN("Failed to close %s", path); @@ -4151,6 +4175,7 @@ qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *path, + bool start_paused, bool bypass_cache) { virDomainDefPtr def = NULL; @@ -4181,7 +4206,8 @@ qemuDomainObjRestore(virConnectPtr conn, virDomainObjAssignDef(vm, def, true); def = NULL; - ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, + start_paused); if (virFileDirectFdClose(directFd) < 0) VIR_WARN("Failed to close %s", path); @@ -4459,7 +4485,7 @@ qemuDomainObjStart(virConnectPtr conn, if (virFileExists(managed_save)) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save, - bypass_cache); + start_paused, bypass_cache); if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); @@ -4475,8 +4501,16 @@ qemuDomainObjStart(virConnectPtr conn, virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_BOOTED); - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (start_paused) { + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + if (event) + qemuDomainEventQueue(driver, event); + } + } } cleanup: @@ -8744,6 +8778,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainEventPtr event = NULL; + virDomainEventPtr event2 = NULL; qemuDomainObjPrivatePtr priv; int rc; @@ -8800,6 +8835,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); if (snap->def->state == VIR_DOMAIN_PAUSED) { /* qemu unconditionally starts the domain running again after * loadvm, so let's pause it to keep consistency @@ -8810,14 +8848,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); } else { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); } - - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, @@ -8860,8 +8897,11 @@ cleanup: } else if (snap) { snap->def->current = false; } - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); + } if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.7.4.4

Implement the new running/paused overrides for saved state management. Unfortunately, for virDomainSaveImageDefineXML, the saved state updates are write-only - I don't know of any way to expose a way to query the current run/pause setting of an existing save image file to the user without adding a new API or modifying the domain xml of virDomainSaveImageGetXMLDesc to include a new element to reflect the state bit encoded into the save image. However, I don't think this is a show-stopper, since the API is designed to leave the state bit alone unless an explicit flag is used to change it. * src/qemu/qemu_driver.c (qemuDomainSaveInternal) (qemuDomainSaveImageOpen): Adjust signature. (qemuDomainSaveFlags, qemuDomainManagedSave) (qemuDomainRestoreFlags, qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML, qemuDomainObjRestore): Adjust callers. --- src/qemu/qemu_driver.c | 68 ++++++++++++++++++++++++++++++++--------------- 1 files changed, 46 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6becd31..4ae6128 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2213,7 +2213,7 @@ qemuCompressProgramName(int compress) static int qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed, bool bypass_cache, const char *xmlin) + int compressed, const char *xmlin, unsigned int flags) { char *xml = NULL; struct qemud_save_header header; @@ -2232,6 +2232,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, gid_t gid = getgid(); int directFlag = 0; virFileDirectFdPtr directFd = NULL; + bool bypass_cache = flags & VIR_DOMAIN_SAVE_BYPASS_CACHE; if (qemuProcessAutoDestroyActive(driver, vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, @@ -2267,6 +2268,11 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, goto endjob; } } + /* libvirt.c already guaranteed these two flags are exclusive. */ + if (flags & VIR_DOMAIN_SAVE_RUNNING) + header.was_running = 1; + else if (flags & VIR_DOMAIN_SAVE_PAUSED) + header.was_running = 0; /* Get XML for the domain. Restore needs only the inactive xml, * including secure. We should get the same result whether xmlin @@ -2508,7 +2514,9 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, int ret = -1; virDomainObjPtr vm = NULL; - virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | + VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); qemuDriverLock(driver); @@ -2546,8 +2554,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, } ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - dxml); + dxml, flags); vm = NULL; cleanup: @@ -2585,7 +2592,9 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) int ret = -1; int compressed; - virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | + VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2616,8 +2625,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) compressed = QEMUD_SAVE_FORMAT_RAW; ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - NULL); + NULL, flags); vm = NULL; cleanup: @@ -3766,15 +3774,16 @@ cleanup: return ret; } -/* Return -1 on failure, -2 if edit was specified but xmlin does not - * represent any changes, and opened fd on all other success. */ +/* Return -1 on failure, -2 if edit was specified but xmlin and state + * (-1 for no change, 0 for paused, 1 for running) do not represent + * any changes, and opened fd on all other success. */ static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin, bool edit) + const char *xmlin, int state, bool edit) { int fd; struct qemud_save_header header; @@ -3854,7 +3863,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, goto error; } - if (edit && STREQ(xml, xmlin)) { + if (edit && STREQ(xml, xmlin) && + (state < 0 || state == header.was_running)) { VIR_FREE(xml); if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close file: %s"), path); @@ -3862,6 +3872,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, } return -2; } + if (state >= 0) + header.was_running = state; /* Create a domain from this XML */ if (!(def = virDomainDefParseString(driver->caps, xml, @@ -4024,14 +4036,22 @@ qemuDomainRestoreFlags(virConnectPtr conn, int ret = -1; struct qemud_save_header header; virFileDirectFdPtr directFd = NULL; + int state = -1; - virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | + VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); qemuDriverLock(driver); + if (flags & VIR_DOMAIN_SAVE_RUNNING) + state = 1; + else if (flags & VIR_DOMAIN_SAVE_PAUSED) + state = 0; + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml, false); + &directFd, dxml, state, false); if (fd < 0) goto cleanup; @@ -4094,7 +4114,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - NULL, false); + NULL, -1, false); if (fd < 0) goto cleanup; @@ -4119,13 +4139,20 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, struct qemud_save_header header; char *xml = NULL; size_t len; + int state = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); qemuDriverLock(driver); + if (flags & VIR_DOMAIN_SAVE_RUNNING) + state = 1; + else if (flags & VIR_DOMAIN_SAVE_PAUSED) + state = 0; + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - dxml, true); + dxml, state, true); if (fd < 0) { /* Check for special case of no change needed. */ @@ -4150,11 +4177,8 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, goto cleanup; } - if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) { - virReportSystemError(errno, _("cannot seek in '%s'"), path); - goto cleanup; - } - if (safewrite(fd, xml, len) != len || + if (safewrite(fd, &header, sizeof(header) != sizeof(header)) || + safewrite(fd, xml, len) != len || VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("failed to write xml to '%s'"), path); goto cleanup; @@ -4185,7 +4209,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL, false); + bypass_cache, &directFd, NULL, -1, false); if (fd < 0) goto cleanup; -- 1.7.4.4

Pretty straight-forward exposure of new flags. For most commands, we let the API reject mutually exclusive flags; but for save-image-edit, we do the sanity check ourselves to avoid looping on flag failure if the edit cycle is ever enhanced to allow the user to retry an edit to fix up an xml validation error. * tools/virsh.c (cmdManagedSave, cmdRestore, cmdSave) (cmdSaveImageDefine, cmdSaveImageEdit): Add new flags. * tools/virsh.pod (managedsave, restore, save, save-image-define) (save-image-edit): Document them. --- tools/virsh.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 15b9bdd..4426d2b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1605,6 +1605,8 @@ static const vshCmdOptDef opts_save[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to save the data")}, {"xml", VSH_OT_STRING, 0, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1627,6 +1629,10 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { vshError(ctl, "%s", _("malformed xml argument")); @@ -1714,6 +1720,8 @@ static const vshCmdOptDef opts_save_image_define[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("saved state file to modify")}, {"xml", VSH_OT_STRING, VSH_OFLAG_REQ, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1724,6 +1732,12 @@ cmdSaveImageDefine(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *xmlfile = NULL; char *xml = NULL; + int flags = 0; + + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1763,6 +1777,8 @@ static const vshCmdInfo info_save_image_edit[] = { static const vshCmdOptDef opts_save_image_edit[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("saved state file to edit")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1774,7 +1790,22 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) char *tmp = NULL; char *doc = NULL; char *doc_edited = NULL; - int flags = VIR_DOMAIN_XML_SECURE; + int getxml_flags = VIR_DOMAIN_XML_SECURE; + int define_flags = 0; + + if (vshCommandOptBool(cmd, "running")) + define_flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + define_flags |= VIR_DOMAIN_SAVE_PAUSED; + + /* Normally, we let the API reject mutually exclusive flags. + * However, in the edit cycle, we let the user retry if the define + * step fails, but the define step will always fail on invalid + * flags, so we reject it up front to avoid looping. */ + if (define_flags == (VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED)) { + vshError(ctl, "%s", _("--running and --saved are mutually exclusive")); + return false; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1783,7 +1814,7 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) return false; /* Get the XML configuration of the saved image. */ - doc = virDomainSaveImageGetXMLDesc(ctl->conn, file, flags); + doc = virDomainSaveImageGetXMLDesc(ctl->conn, file, getxml_flags); if (!doc) goto cleanup; @@ -1801,8 +1832,9 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) if (!doc_edited) goto cleanup; - /* Compare original XML with edited. Has it changed at all? */ - if (STREQ(doc, doc_edited)) { + /* Compare original XML with edited. Short-circuit if it did not + * change, and we do not have any flags. */ + if (STREQ(doc, doc_edited) && !define_flags) { vshPrint(ctl, _("Saved image %s XML configuration not changed.\n"), file); ret = true; @@ -1810,7 +1842,8 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) } /* Everything checks out, so redefine the xml. */ - if (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, 0) < 0) { + if (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, + define_flags) < 0) { vshError(ctl, _("Failed to update %s"), file); goto cleanup; } @@ -1843,6 +1876,8 @@ static const vshCmdInfo info_managedsave[] = { static const vshCmdOptDef opts_managedsave[] = { {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on next start")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on next start")}, {NULL, 0, 0, NULL} }; @@ -1859,6 +1894,10 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -2202,6 +2241,8 @@ static const vshCmdOptDef opts_restore[] = { N_("avoid file system cache when restoring")}, {"xml", VSH_OT_STRING, 0, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("restore domain into running state")}, + {"paused", VSH_OT_BOOL, 0, N_("restore domain into paused state")}, {NULL, 0, 0, NULL} }; @@ -2222,6 +2263,10 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { vshError(ctl, "%s", _("malformed xml argument")); diff --git a/tools/virsh.pod b/tools/virsh.pod index 81d7a1e..5967d1f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,6 +546,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. =item B<managedsave> I<domain-id> [I<--bypass-cache>] +[{I<--running> | I<--paused>}] Save and destroy (stop) a running domain, so it can be restarted from the same state at a later time. When the virsh B<start> command is next run for @@ -553,6 +554,11 @@ the domain, it will automatically be started from this saved state. If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. +Normally, starting a managed save will decide between running or paused +based on the state the domain was in when the save was done; passing +either the I<--running> or I<--paused> flag will allow overriding which +state the B<start> should use. + The B<dominfo> command can be used to query whether a domain currently has any managed save image. @@ -635,6 +641,7 @@ The exact behavior of a domain when it reboots is set by the I<on_reboot> parameter in the domain's XML definition. =item B<restore> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] +[{I<--running> | I<--paused>}] Restores a domain from a B<virsh save> state file. See I<save> for more info. @@ -647,12 +654,18 @@ in the host-specific portions of the domain XML. For example, it can be used to account for file naming differences in underlying storage due to disk snapshots taken after the guest was saved. +Normally, restoring a saved image will use the state recorded in the +save image to decide between running or paused; passing either the +I<--running> or I<--paused> flag will allow overriding which state the +domain should be started in. + B<Note>: To avoid corrupting file system contents within the domain, you should not reuse the saved state file for a second B<restore> unless you have also reverted all storage volumes back to the same contents as when the state file was created. =item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] +[{I<--running> | I<--paused>}] Saves a running domain (RAM, but not disk state) to a state file so that it can be restored @@ -672,12 +685,17 @@ in the host-specific portions of the domain XML. For example, it can be used to account for file naming differences that are planned to be made via disk snapshots of underlying storage after the guest is saved. +Normally, restoring a saved image will decide between running or paused +based on the state the domain was in when the save was done; passing +either the I<--running> or I<--paused> flag will allow overriding which +state the B<restore> should use. + Domain saved state files assume that disk images will be unchanged between the creation and restore point. For a more complete system restore point, where the disk state is saved alongside the memory state, see the B<snapshot> family of commands. -=item B<save-image-define> I<file> I<xml> +=item B<save-image-define> I<file> I<xml> [{I<--running> | I<--paused>}] Update the domain XML that will be used when I<file> is later used in the B<restore> command. The I<xml> argument must be a file @@ -686,17 +704,27 @@ host-specific portions of the domain XML. For example, it can be used to account for file naming differences resulting from creating disk snapshots of underlying storage after the guest was saved. +The save image records whether the domain should be restored to a +running or paused state. Normally, this command does not alter the +recorded state; passing either the I<--running> or I<--paused> flag +will allow overriding which state the B<restore> should use. + =item B<save-image-dumpxml> I<file> [I<--security-info>] Extract the domain XML that was in effect at the time the saved state file I<file> was created with the B<save> command. Using I<--security-info> will also include security sensitive information. -=item B<save-image-edit> I<file> +=item B<save-image-edit> I<file> [{I<--running> | I<--paused>}] Edit the XML configuration associated with a saved state file I<file> created by the B<save> command. +The save image records whether the domain should be restored to a +running or paused state. Normally, this command does not alter the +recorded state; passing either the I<--running> or I<--paused> flag +will allow overriding which state the B<restore> should use. + This is equivalent to: virsh save-image-dumpxml state-file > state-file.xml -- 1.7.4.4

Commit 6766ff10 introduced a corner case bug with snapshot creation: if a snapshot is created, but then we hit OOM while trying to create the return value of the function, then we have polluted the internal directory with the snapshot metadata with no way to clean it up from the running libvirtd. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Don't write metadata file on OOM condition. --- src/qemu/qemu_driver.c | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cce11c3..77b7609 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8583,18 +8583,21 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, * do; we've successfully taken the snapshot, and we are now running * on it, so we have to go forward the best we can */ - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0) - goto cleanup; - vm->current_snapshot = snap; snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: if (vm) { - if (snapshot) - vm->current_snapshot = snap; - else if (snap) + if (snapshot) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->snapshotDir) < 0) + VIR_WARN("unable to save metadata for snapshot %s", + snap->def->name); + else + vm->current_snapshot = snap; + } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); + } virDomainObjUnlock(vm); } virDomainSnapshotDefFree(def); -- 1.7.4.4

qemuDomainSnapshotRevertInactive has the same FIXMEs as qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly handle partial loop iterations should be applied later to both functions, but we're not making the situation any worse in this patch. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use qemu-img rather than 'qemu -loadvm' to revert to offline snapshot. (qemuDomainSnapshotRevertInactive): New helper. --- src/qemu/qemu_driver.c | 65 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7950aff..1caa870 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8738,6 +8738,52 @@ cleanup: return xml; } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotRevertInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-a", 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) + goto cleanup; + } + } + + ret = 0; + +cleanup: + VIR_FREE(qemuimgarg[0]); + return ret; +} + static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -8844,14 +8890,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } } else { - /* qemu is a little funny with running guests and the restoration - * of snapshots. If the snapshot was taken online, - * then after a "loadvm" monitor command, the VM is set running - * again. If the snapshot was taken offline, then after a "loadvm" - * monitor command the VM is left paused. Unpausing it leads to - * the memory state *before* the loadvm with the disk *after* the - * loadvm, which obviously is bound to corrupt something. - * Therefore we destroy the domain and set it to "off" in this case. + /* Newer qemu -loadvm refuses to revert to the state of a snapshot + * created by qemu-img snapshot -c. If the domain is running, we + * must take it offline; then do the revert using qemu-img. */ if (virDomainObjIsActive(vm)) { @@ -8861,6 +8902,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { + /* XXX it should be impossible to create an offline snapshot + * of a transient domain. Once we fix 'undefine' to convert + * a defined domain back to transient, that transition should + * be rejected if any offline snapshots exist. For now, we + * just stop the transient domain and quit, without reverting + * any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; @@ -8868,7 +8915,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } - if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) goto endjob; } -- 1.7.4.4

For a system checkpoint of a running or paused domain, it's fairly easy to honor new flags for altering which state to use after the revert. For an inactive snapshot, the revert has to be done while there is no qemu process, so do back-to-back transitions; this also lets us revert to inactive snapshots even for transient domains. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Support new flags. --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6f6447e..094455f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8857,7 +8857,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); /* We have the following transitions, which create the following events: * 1. inactive -> inactive: none @@ -8889,6 +8890,17 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + if (!vm->persistent && + snap->def->state != VIR_DOMAIN_RUNNING && + snap->def->state != VIR_DOMAIN_PAUSED && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domain needs to request run or pause " + "to revert to inactive snapshot")); + goto cleanup; + } + if (vm->current_snapshot) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, @@ -8962,7 +8974,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } /* Touch up domain state. */ - if (snap->def->state == VIR_DOMAIN_PAUSED) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && + (snap->def->state == VIR_DOMAIN_PAUSED || + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); @@ -9015,20 +9029,49 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, detail); + } + + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) { if (!vm->persistent) { - /* XXX For transient domains, enforce that one of the - * flags is set to get domain running again. For now, - * reverting to offline on a transient domain ends up - * killing the domain, without reverting any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; goto cleanup; } + goto endjob; } - if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) - goto endjob; + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + /* Flush first event, now do transition 2 or 3 */ + bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; + + if (event) + qemuDomainEventQueue(driver, event); + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, + paused, false, -1, NULL, NULL, + VIR_VM_OP_CREATE); + virDomainAuditStart(vm, "from-snapshot", rc >= 0); + if (rc < 0) { + if (!vm->persistent) { + if (qemuDomainObjEndJob(driver, vm) > 0) + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + goto cleanup; + } + goto endjob; + } + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + if (paused) { + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + } + } } ret = 0; -- 1.7.4.4

Changing the current vm, and writing that change to the file system, all before a new qemu starts, is risky; it's hard to roll back if starting the new qemu fails for some reason. Instead of abusing vm->current_snapshot and making the command line generator decide whether the current snapshot warrants using -loadvm, it is better to just directly pass a snapshot all the way through the call chain if it is to be loaded. This frees up the last use of snapshot->def->active for qemu's use, so the next patch can repurpose that field for tracking which snapshot is current. * src/qemu/qemu_command.c (qemuBuildCommandLine): Don't use active field of snapshot. * src/qemu/qemu_process.c (qemuProcessStart): Add a parameter. * src/qemu/qemu_process.h (qemuProcessStart): Update prototype. * src/qemu/qemu_migration.c (qemuMigrationPrepareAny): Update callers. * src/qemu/qemu_driver.c (qemudDomainCreate) (qemuDomainSaveImageStartVM, qemuDomainObjStart) (qemuDomainRevertToSnapshot): Likewise. (qemuDomainSnapshotSetCurrentActive) (qemuDomainSnapshotSetCurrentInactive): Delete unused functions. --- src/qemu/qemu_command.c | 7 ++--- src/qemu/qemu_driver.c | 47 ++++---------------------------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 10 +------- src/qemu/qemu_process.h | 1 + 5 files changed, 12 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 287ad8d..61fc2c7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2866,7 +2866,7 @@ qemuBuildCommandLine(virConnectPtr conn, virBitmapPtr qemuCaps, const char *migrateFrom, int migrateFd, - virDomainSnapshotObjPtr current_snapshot, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop) { int i; @@ -4782,9 +4782,8 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (current_snapshot && current_snapshot->def->active) - virCommandAddArgList(cmd, "-loadvm", current_snapshot->def->name, - NULL); + if (snapshot) + virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); if (def->namespaceData) { qemuDomainCmdlineDefPtr qemucmd; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d712bf2..c4183de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -804,12 +804,6 @@ qemudShutdown(void) { } -static int qemuDomainSnapshotSetCurrentActive(virDomainObjPtr vm, - char *snapshotDir); -static int qemuDomainSnapshotSetCurrentInactive(virDomainObjPtr vm, - char *snapshotDir); - - static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -1297,7 +1291,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuProcessStart(conn, driver, vm, NULL, (flags & VIR_DOMAIN_START_PAUSED) != 0, (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, - -1, NULL, VIR_VM_OP_CREATE) < 0) { + -1, NULL, NULL, VIR_VM_OP_CREATE) < 0) { virDomainAuditStart(vm, "booted", false); if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, @@ -3920,7 +3914,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, /* Set the migration source and start it up. */ ret = qemuProcessStart(conn, driver, vm, "stdio", true, - false, *fd, path, VIR_VM_OP_RESTORE); + false, *fd, path, NULL, VIR_VM_OP_RESTORE); if (intermediatefd != -1) { if (ret < 0) { @@ -4462,7 +4456,7 @@ qemuDomainObjStart(virConnectPtr conn, } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, - autodestroy, -1, NULL, VIR_VM_OP_CREATE); + autodestroy, -1, NULL, NULL, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "booted", ret >= 0); if (ret >= 0) { virDomainEventPtr event = @@ -8318,32 +8312,6 @@ cleanup: return ret; } -static int qemuDomainSnapshotSetCurrentActive(virDomainObjPtr vm, - char *snapshotDir) -{ - if (vm->current_snapshot) { - vm->current_snapshot->def->active = 1; - - return qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, - snapshotDir); - } - - return 0; -} - -static int qemuDomainSnapshotSetCurrentInactive(virDomainObjPtr vm, - char *snapshotDir) -{ - if (vm->current_snapshot) { - vm->current_snapshot->def->active = 0; - - return qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, - snapshotDir); - } - - return 0; -} - static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { @@ -8861,17 +8829,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } } else { - if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) - goto endjob; - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - true, false, -1, NULL, VIR_VM_OP_CREATE); + true, false, -1, NULL, vm->current_snapshot, + VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) goto endjob; - if (qemuDomainSnapshotSetCurrentInactive(vm, - driver->snapshotDir) < 0) - goto endjob; } /* Touch up domain state. */ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..b88d9c0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1034,7 +1034,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, * -incoming $migrateFrom */ if (qemuProcessStart(dconn, driver, vm, migrateFrom, true, - true, dataFD[0], NULL, + true, dataFD[0], NULL, NULL, VIR_VM_OP_MIGRATE_IN_START) < 0) { virDomainAuditStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 616c8e2..f691bbb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2614,6 +2614,7 @@ int qemuProcessStart(virConnectPtr conn, bool autodestroy, int stdin_fd, const char *stdin_path, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop) { int ret; @@ -2816,16 +2817,9 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, - migrateFrom, stdin_fd, - vm->current_snapshot, vmop))) + migrateFrom, stdin_fd, snapshot, vmop))) goto cleanup; -#if 0 - /* XXX */ - if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) - goto cleanup; -#endif - /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { char *xml = virDomainDefFormat(vm->def, 0); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index e9b910d..96ba3f3 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -52,6 +52,7 @@ int qemuProcessStart(virConnectPtr conn, bool autodestroy, int stdin_fd, const char *stdin_path, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop); void qemuProcessStop(struct qemud_driver *driver, -- 1.7.4.4

Audit all changes to the qemu vm->current_snapshot, and make them update the saved xml file for both the previous and the new snapshot, so that there is always at most one snapshot with <active>1</active> in the xml, and that snapshot is used as the current snapshot even across libvirtd restarts. * src/conf/domain_conf.h (_virDomainSnapshotDef): Alter member type and name. * src/conf/domain_conf.c (virDomainSnapshotDefParseString) (virDomainSnapshotDefFormat): Update clients. * docs/schemas/domainsnapshot.rng: Tighten rng. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Reload current snapshot. (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainSnapshotDiscard): Track current snapshot. --- docs/schemas/domainsnapshot.rng | 5 ++- src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 4 +- src/qemu/qemu_driver.c | 81 +++++++++++++++++++++++++++++---------- 4 files changed, 72 insertions(+), 24 deletions(-) diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 86bab0b..410833f 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -29,7 +29,10 @@ </optional> <optional> <element name='active'> - <text/> + <choice> + <value>0</value> + <value>1</value> + </choice> </element> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0a2c9eb..44212cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10960,6 +10960,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virDomainSnapshotDefPtr ret = NULL; char *creation = NULL, *state = NULL; struct timeval tv; + int active; xml = virXMLParseCtxt(NULL, xmlStr, "domainsnapshot.xml", &ctxt); if (!xml) { @@ -11016,11 +11017,12 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, goto cleanup; } - if (virXPathLong("string(./active)", ctxt, &def->active) < 0) { + if (virXPathInt("string(./active)", ctxt, &active) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find 'active' element")); goto cleanup; } + def->current = active != 0; } else def->creationTime = tv.tv_sec; @@ -11062,7 +11064,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); virBufferAddLit(&buf, " </domain>\n"); if (internal) - virBufferAsprintf(&buf, " <active>%ld</active>\n", def->active); + virBufferAsprintf(&buf, " <active>%d</active>\n", def->current); virBufferAddLit(&buf, "</domainsnapshot>\n"); if (virBufferError(&buf)) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2cc9b06..8382d28 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1297,13 +1297,15 @@ enum virDomainTaintFlags { typedef struct _virDomainSnapshotDef virDomainSnapshotDef; typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; struct _virDomainSnapshotDef { + /* Public XML. */ char *name; char *description; char *parent; long long creationTime; /* in seconds */ int state; - long active; + /* Internal use. */ + bool current; /* At most one snapshot in the list should have this set */ }; typedef struct _virDomainSnapshotObj virDomainSnapshotObj; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4183de..b492c97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -294,6 +294,7 @@ static void qemuDomainSnapshotLoad(void *payload, char *fullpath; virDomainSnapshotDefPtr def = NULL; virDomainSnapshotObjPtr snap = NULL; + virDomainSnapshotObjPtr current = NULL; char ebuf[1024]; virDomainObjLock(vm); @@ -339,7 +340,8 @@ static void qemuDomainSnapshotLoad(void *payload, def = virDomainSnapshotDefParseString(xmlStr, 0); if (def == NULL) { /* Nothing we can do here, skip this one */ - VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), fullpath); + VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), + fullpath); VIR_FREE(fullpath); VIR_FREE(xmlStr); continue; @@ -348,12 +350,22 @@ static void qemuDomainSnapshotLoad(void *payload, snap = virDomainSnapshotAssignDef(&vm->snapshots, def); if (snap == NULL) { virDomainSnapshotDefFree(def); + } else if (snap->def->current) { + current = snap; + if (!vm->current_snapshot) + vm->current_snapshot = snap; } VIR_FREE(fullpath); VIR_FREE(xmlStr); } + if (vm->current_snapshot != current) { + VIR_ERROR(_("Too many snapshots claiming to be current for domain %s"), + vm->def->name); + vm->current_snapshot = NULL; + } + /* FIXME: qemu keeps internal track of snapshots. We can get access * to this info via the "info snapshots" monitor command for running * domains, or via "qemu-img snapshot -l" for shutoff domains. It would @@ -8477,12 +8489,17 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, def = NULL; snap->def->state = virDomainObjGetState(vm, NULL); + snap->def->current = true; if (vm->current_snapshot) { snap->def->parent = strdup(vm->current_snapshot->def->name); if (snap->def->parent == NULL) { virReportOOMError(); goto cleanup; } + vm->current_snapshot->def->current = false; + if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, + driver->snapshotDir) < 0) + goto cleanup; vm->current_snapshot = NULL; } @@ -8502,6 +8519,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, */ if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0) goto cleanup; + vm->current_snapshot = snap; snapshot = virGetDomainSnapshot(domain, snap->def->name); @@ -8788,7 +8806,17 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - vm->current_snapshot = snap; + if (vm->current_snapshot) { + vm->current_snapshot->def->current = false; + if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, + driver->snapshotDir) < 0) + goto cleanup; + vm->current_snapshot = NULL; + /* XXX Should we restore vm->current_snapshot after this point + * in the failure cases where we know there was no change? */ + } + + snap->def->current = true; if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -8830,7 +8858,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } else { rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - true, false, -1, NULL, vm->current_snapshot, + true, false, -1, NULL, snap, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) @@ -8894,6 +8922,15 @@ endjob: vm = NULL; cleanup: + if (vm && ret == 0) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->snapshotDir) < 0) + ret = -1; + else + vm->current_snapshot = snap; + } else if (snap) { + snap->def->current = false; + } if (event) qemuDomainEventQueue(driver, event); if (vm) @@ -8912,7 +8949,7 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, int ret = -1; int i; qemuDomainObjPrivatePtr priv; - virDomainSnapshotObjPtr parentsnap; + virDomainSnapshotObjPtr parentsnap = NULL; if (!virDomainObjIsActive(vm)) { qemuimgarg[0] = qemuFindQemuImgBinary(); @@ -8951,31 +8988,35 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, qemuDomainObjExitMonitorWithDriver(driver, vm); } + if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, + vm->def->name, snap->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + if (snap == vm->current_snapshot) { if (snap->def->parent) { parentsnap = virDomainSnapshotFindByName(&vm->snapshots, snap->def->parent); if (!parentsnap) { - qemuReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, - _("no domain snapshot parent with matching name '%s'"), - snap->def->parent); - goto cleanup; + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent); + } else { + parentsnap->def->current = true; + if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, + driver->snapshotDir) < 0) { + VIR_WARN("failed to set parent snapshot '%s' as current", + snap->def->parent); + parentsnap->def->current = false; + parentsnap = NULL; + } } - - /* Now we set the new current_snapshot for the domain */ - vm->current_snapshot = parentsnap; - } else { - vm->current_snapshot = NULL; } + vm->current_snapshot = parentsnap; } - if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, - vm->def->name, snap->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - unlink(snapFile); - + if (unlink(snapFile) < 0) + VIR_WARN("Failed to unlink %s", snapFile); virDomainSnapshotObjListRemove(&vm->snapshots, snap); ret = 0; -- 1.7.4.4

On Wed, Aug 24, 2011 at 09:22:24AM -0600, Eric Blake wrote: 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 :|

Each snapshot lookup was iterating over the entire hash table, O(n), instead of honing in directly on the hash key, amortized O(1). Besides, fixing this means that virDomainSnapshotFindByName can now be used inside another virHashForeach iteration (without this patch, attempts to lookup a snapshot by name during a hash iteration will fail due to nested iteration). * src/conf/domain_conf.c (virDomainSnapshotFindByName): Simplify. (virDomainSnapshotObjListSearchName): Delete unused function. --- This patch is necessary prior to patch 8/43 to fix the recursive iteration bug reported by Dan. The actual patch 8/43 does not have to change! src/conf/domain_conf.c | 20 ++++---------------- 1 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44212cf..6f19b6c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11213,23 +11213,11 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots) return count; } -static int virDomainSnapshotObjListSearchName(const void *payload, - const void *name ATTRIBUTE_UNUSED, - const void *data) -{ - virDomainSnapshotObjPtr obj = (virDomainSnapshotObjPtr)payload; - int want = 0; - - if (STREQ(obj->def->name, (const char *)data)) - want = 1; - - return want; -} - -virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, - const char *name) +virDomainSnapshotObjPtr +virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, + const char *name) { - return virHashSearch(snapshots->objs, virDomainSnapshotObjListSearchName, name); + return virHashLookup(snapshots->objs, name); } void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, -- 1.7.4.4

Similar to the last patch in isolating the filtering from the client actions, so that clients don't have to reinvent the filtering. * src/conf/domain_conf.h (virDomainSnapshotForEachChild): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainSnapshotActOnChild) (virDomainSnapshotForEachChild): New functions. (virDomainSnapshotCountChildren): Delete. (virDomainSnapshotHasChildren): Simplify. * src/qemu/qemu_driver.c (qemuDomainSnapshotReparentChildren) (qemuDomainSnapshotDelete): Likewise. --- src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++------------- src/conf/domain_conf.h | 4 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 31 ++++++++++++++--------------- 4 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 75fa7f7..01bbd04 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11238,32 +11238,52 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virHashRemoveEntry(snapshots->objs, snapshot->def->name); } -struct snapshot_has_children { - char *name; +struct snapshot_act_on_child { + char *parent; int number; + virHashIterator iter; + void *data; }; -static void virDomainSnapshotCountChildren(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) +static void +virDomainSnapshotActOnChild(void *payload, + const void *name, + void *data) { virDomainSnapshotObjPtr obj = payload; - struct snapshot_has_children *curr = data; + struct snapshot_act_on_child *curr = data; - if (obj->def->parent && STREQ(obj->def->parent, curr->name)) + if (obj->def->parent && STREQ(curr->parent, obj->def->parent)) { curr->number++; + if (curr->iter) + (curr->iter)(payload, name, curr->data); + } } -int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, - virDomainSnapshotObjListPtr snapshots) +/* Run iter(data) on all direct children of snapshot, while ignoring all + * other entries in snapshots. Return the number of children + * visited. No particular ordering is guaranteed. */ +int +virDomainSnapshotForEachChild(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data) { - struct snapshot_has_children children; + struct snapshot_act_on_child act; - children.name = snap->def->name; - children.number = 0; - virHashForEach(snapshots->objs, virDomainSnapshotCountChildren, &children); + act.parent = snapshot->def->name; + act.number = 0; + act.iter = iter; + act.data = data; + virHashForEach(snapshots->objs, virDomainSnapshotActOnChild, &act); - return children.number; + return act.number; +} + +int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, + virDomainSnapshotObjListPtr snapshots) +{ + return virDomainSnapshotForEachChild(snapshots, snap, NULL, NULL); } typedef enum { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d266605..5f752ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1343,6 +1343,10 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, virDomainSnapshotObjListPtr snapshots); +int virDomainSnapshotForEachChild(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); int virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot, virHashIterator iter, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fcce810..74c5ff4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -384,6 +384,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefParseString; virDomainSnapshotFindByName; +virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotHasChildren; virDomainSnapshotObjListGetNames; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1405b71..5101167 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9050,7 +9050,7 @@ qemuDomainSnapshotDiscardDescendant(void *payload, struct snap_reparent { struct qemud_driver *driver; - virDomainSnapshotObjPtr snap; + const char *parent; virDomainObjPtr vm; int err; }; @@ -9067,22 +9067,20 @@ qemuDomainSnapshotReparentChildren(void *payload, return; } - if (snap->def->parent && STREQ(snap->def->parent, rep->snap->def->name)) { - VIR_FREE(snap->def->parent); + VIR_FREE(snap->def->parent); - if (rep->snap->def->parent != NULL) { - snap->def->parent = strdup(rep->snap->def->parent); + if (rep->parent != NULL) { + snap->def->parent = strdup(rep->parent); - if (snap->def->parent == NULL) { - virReportOOMError(); - rep->err = -1; - return; - } + if (snap->def->parent == NULL) { + virReportOOMError(); + rep->err = -1; + return; } - - rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, - rep->driver->snapshotDir); } + + rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, + rep->driver->snapshotDir); } static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, @@ -9130,11 +9128,12 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto endjob; } else { rep.driver = driver; - rep.snap = snap; + rep.parent = snap->def->parent; rep.vm = vm; rep.err = 0; - virHashForEach(vm->snapshots.objs, qemuDomainSnapshotReparentChildren, - &rep); + virDomainSnapshotForEachChild(&vm->snapshots, snap, + qemuDomainSnapshotReparentChildren, + &rep); if (rep.err < 0) goto endjob; } -- 1.7.4.4

To make it easier to know when undefine will fail because of existing snapshot metadata, we need to know how many snapshots have metadata. Also, it is handy to filter the list of snapshots to just those that have no parents; document that flag now, but implement it in later patches. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_LIST_ROOTS) (VIR_DOMAIN_SNAPSHOT_LIST_METADATA): New flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotListNames): Document them. * src/esx/esx_driver.c (esxDomainSnapshotNum) (esxDomainSnapshotListNames): Implement trivial flag. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotNum) (vboxDomainSnapshotListNames): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotNum) (qemuDomainSnapshotListNames): Likewise. --- include/libvirt/libvirt.h.in | 9 +++++++++ src/esx/esx_driver.c | 10 +++++++--- src/libvirt.c | 27 ++++++++++++++++++++++----- src/qemu/qemu_driver.c | 8 ++++++-- src/vbox/vbox_tmpl.c | 15 +++++++++++++-- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eae0a10..20fdbdf 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2551,6 +2551,15 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags); +/* Flags valid for both virDomainSnapshotNum() and + * virDomainSnapshotListNames(). */ +typedef enum { + VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots which + have no parents */ + VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots which + have metadata */ +} virDomainSnapshotListFlags; + /* Return the number of snapshots for this domain */ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index beeafbd..dbc7694 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4329,12 +4329,16 @@ esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags) esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); if (esxVI_EnsureSession(priv->primary) < 0) { return -1; } + /* ESX snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) + return 0; + if (esxVI_LookupRootSnapshotTreeList(priv->primary, domain->uuid, &rootSnapshotTreeList) < 0) { return -1; @@ -4357,14 +4361,14 @@ esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); if (names == NULL || nameslen < 0) { ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); return -1; } - if (nameslen == 0) { + if (nameslen == 0 || (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA)) { return 0; } diff --git a/src/libvirt.c b/src/libvirt.c index a80c140..4034d4e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15532,11 +15532,19 @@ error: /** * virDomainSnapshotNum: * @domain: a domain object - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-or of supported virDomainSnapshotListFlags + * + * Provides the number of domain snapshots for this domain. * - * Provides the number of domain snapshots for this domain.. + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is + * filtered to the number of snapshots that have no parents. * - * Returns the number of domain snapshost found or -1 in case of error. + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. + * + * Returns the number of domain snapshots found or -1 in case of error. */ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags) @@ -15572,11 +15580,20 @@ error: * @domain: a domain object * @names: array to collect the list of names of snapshots * @nameslen: size of @names - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-or of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots for the given domain, and store * their names in @names. Caller is responsible for freeing each member - * of the array. + * of the array. The value to use for @nameslen can be determined by + * virDomainSnapshotNum() with the same @flags. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is + * filtered to the number of snapshots that have no parents. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. * * Returns the number of domain snapshots found or -1 in case of error. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b59a35..9dd1ceb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8544,7 +8544,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8572,7 +8572,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8584,6 +8584,10 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, goto cleanup; } + /* All qemu snapshots have libvirt metadata, so + * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our + * answer. */ + n = virDomainSnapshotObjListNum(&vm->snapshots); cleanup: diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 8de2bae..fc9739e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5861,7 +5861,7 @@ vboxDomainSnapshotNum(virDomainPtr dom, nsresult rc; PRUint32 snapshotCount; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine); @@ -5871,6 +5871,12 @@ vboxDomainSnapshotNum(virDomainPtr dom, goto cleanup; } + /* VBox snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + ret = 0; + goto cleanup; + } + rc = machine->vtbl->GetSnapshotCount(machine, &snapshotCount); if (NS_FAILED(rc)) { vboxError(VIR_ERR_INTERNAL_ERROR, @@ -5901,7 +5907,7 @@ vboxDomainSnapshotListNames(virDomainPtr dom, int count = 0; int i; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine); @@ -5911,6 +5917,11 @@ vboxDomainSnapshotListNames(virDomainPtr dom, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + ret = 0; + goto cleanup; + } + if ((count = vboxDomainSnapshotGetAll(dom, machine, &snapshots)) < 0) goto cleanup; -- 1.7.4.4

Just as leaving managed save metadata behind can cause problems when creating a new domain that happens to collide with the name of the just-deleted domain, the same is true of leaving any snapshot metadata behind. For safety sake, extend the semantic change of commit b26a9fa9 to also cover snapshot metadata as a reason to reject losing the last reference to a domain (undefine on an inactive, or shutdown/destroy on a transient). The caller must first take care of snapshots, possible via the existing virDomainSnapshotDelete. This also documents some new flags that hypervisors can choose to support to shortcuts taking care of the metadata as part of the shutdown process; however, nontrivial driver support for these flags will be deferred to future patches. Note that ESX and VBox can never be transient; therefore, they do not have to affect shutdown/destroy (the persistent domain still remains); likewise they never store snapshot metadata, so one of the two flags is trivial. The bulk of the nontrivial work remaining is thus in the qemu driver. * include/libvirt/libvirt.h.in (VIR_DOMAIN_UNDEFINE_SNAPSHOTS) (VIR_DOMAIN_DESTROY_SNAPSHOTS): New flags. * src/libvirt.c (virDomainUndefine, virDomainUndefineFlags) (virDomainDestroy, virDomainDestroyFlags, virDomainShutdown): Document new limitations and flags. * src/esx/esx_driver.c (esxDomainUndefineFlags): Enforce the limitations. * src/vbox/vbox_tmpl.c (vboxDomainUndefineFlags): Likewise. * src/qemu/qemu_driver.c (qemuDomainUndefineFlags) (qemuDomainShutdown, qemuDomainDestroyFlags): Likewise. --- include/libvirt/libvirt.h.in | 25 ++++++++++++++++++--- src/esx/esx_driver.c | 11 ++++++++- src/libvirt.c | 47 +++++++++++++++++++++++++++++++++++------ src/qemu/qemu_driver.c | 27 ++++++++++++++++++++++++ src/vbox/vbox_tmpl.c | 11 ++++++++- 5 files changed, 106 insertions(+), 15 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 20fdbdf..36f1b34 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -919,10 +919,20 @@ virConnectPtr virDomainGetConnect (virDomainPtr domain); * Domain creation and destruction */ -/* - * typedef enum { - * } virDomainDestroyFlagsValues; + +/* Counterparts to virDomainUndefineFlagsValues, but note that running + * domains have no managed save data, so no flag is provided for that. */ +typedef enum { + /* VIR_DOMAIN_DESTROY_MANAGED_SAVE = (1 << 0), */ /* Reserved */ + VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, + then also remove any + snapshot metadata */ + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL = (1 << 2), /* If last use of domain, + then also remove any + snapshot data */ +} virDomainDestroyFlagsValues; + virDomainPtr virDomainCreateXML (virConnectPtr conn, const char *xmlDesc, unsigned int flags); @@ -1240,7 +1250,14 @@ virDomainPtr virDomainDefineXML (virConnectPtr conn, int virDomainUndefine (virDomainPtr domain); typedef enum { - VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), + VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), /* Also remove any + managed save */ + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, + then also remove any + snapshot metadata */ + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL = (1 << 2), /* If last use of domain, + then also remove any + snapshot data */ /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index dbc7694..90f55c3 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1950,7 +1950,9 @@ esxDomainDestroyFlags(virDomainPtr domain, esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; - virCheckFlags(0, -1); + /* No transient domains, so these flags are trivially ignored. */ + virCheckFlags(VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL, -1); if (priv->vCenter != NULL) { ctx = priv->vCenter; @@ -3309,7 +3311,9 @@ esxDomainUndefineFlags(virDomainPtr domain, esxVI_String *propertyNameList = NULL; esxVI_VirtualMachinePowerState powerState; - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); if (priv->vCenter != NULL) { ctx = priv->vCenter; @@ -3337,6 +3341,9 @@ esxDomainUndefineFlags(virDomainPtr domain, goto cleanup; } + /* ESX snapshots maintain no metadata, so we can trivially ignore + * that flag. XXX Wire up VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL. */ + if (esxVI_UnregisterVM(ctx, virtualMachine->obj) < 0) { goto cleanup; } diff --git a/src/libvirt.c b/src/libvirt.c index 4034d4e..b0160fb 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2043,6 +2043,10 @@ error: * does not free the associated virDomainPtr object. * This function may require privileged access * + * If the domain is transient and has any snapshot metadata (see + * virDomainSnapshotNum()), then the destroy will fail. See + * virDomainDestroyFlags() for more control. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2092,7 +2096,20 @@ error: * This function may require privileged access. * * Calling this function with no @flags set (equal to zero) - * is equivalent to calling virDomainDestroy. + * is equivalent to calling virDomainDestroy. Using virDomainShutdown() + * may produce cleaner results for the guest's disks, but depends on guest + * support. + * + * If the domain is transient and has any snapshot metadata (see + * virDomainSnapshotNum()), then including + * VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA in @flags will also remove + * that metadata, including VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL will + * remove the metadata and snapshot data contents, and omitting both + * of these flags will cause the destroy process to fail. + * VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA and + * VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL have no effect if the domain is + * persistent, and VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA has no effect + * if libvirt does not maintain snapshot metadata. * * Returns 0 in case of success and -1 in case of failure. */ @@ -2860,12 +2877,16 @@ error: * virDomainShutdown: * @domain: a domain object * - * Shutdown a domain, the domain object is still usable there after but + * Shutdown a domain, the domain object is still usable thereafter but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. + * request. For guests that react to a shutdown request, the differences + * from virDomainDestroy() are that the guests disk storage will be in a + * stable state rather than having the (virtual) power cord pulled, and + * this command returns as soon as the shutdown request is issued rather + * than blocking until the guest is no longer running. * - * TODO: should we add an option for reboot, knowing it may not be doable - * in the general case ? + * If the domain is transient and has any snapshot metadata (see + * virDomainSnapshotNum()), then the shutdown will fail. * * Returns 0 in case of success and -1 in case of failure. */ @@ -6814,8 +6835,9 @@ error: * the domain configuration is removed. * * If the domain has a managed save image (see - * virDomainHasManagedSaveImage()), then the undefine will fail. See - * virDomainUndefineFlags() for more control. + * virDomainHasManagedSaveImage()), or if the domain is active and has any + * snapshot metadata (see virDomainSnapshotNum()), then the undefine will + * fail. See virDomainUndefineFlags() for more control. * * Returns 0 in case of success, -1 in case of error */ @@ -6866,6 +6888,17 @@ error: * then including VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in @flags will also remove * that file, and omitting the flag will cause the undefine process to fail. * + * If the domain is inactive and has any snapshot metadata (see + * virDomainSnapshotNum()), then including + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove + * that metadata, including VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL will + * remove the metadata and snapshot data contents, and omitting both + * of these flags will cause the undefine process to fail. + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA and + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL have no effect if the domain is + * active, and VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA has no effect if + * libvirt does not maintain snapshot metadata. + * * Returns 0 in case of success, -1 in case of error */ int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2cb1d7..6557854 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1472,6 +1472,7 @@ static int qemuDomainShutdown(virDomainPtr dom) { virDomainObjPtr vm; int ret = -1; qemuDomainObjPrivatePtr priv; + int nsnapshots; qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1485,6 +1486,14 @@ static int qemuDomainShutdown(virDomainPtr dom) { goto cleanup; } + if (!vm->persistent && + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete transient domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -1580,6 +1589,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, int ret = -1; virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; + int nsnapshots; virCheckFlags(0, -1); @@ -1593,6 +1603,14 @@ qemuDomainDestroyFlags(virDomainPtr dom, goto cleanup; } + if (!vm->persistent && + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete transient domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + priv = vm->privateData; priv->fakeReboot = false; @@ -4712,6 +4730,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, virDomainEventPtr event = NULL; char *name = NULL; int ret = -1; + int nsnapshots; virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); @@ -4726,6 +4745,14 @@ qemuDomainUndefineFlags(virDomainPtr dom, goto cleanup; } + if (!virDomainObjIsActive(vm) && + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + if (!vm->persistent) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot undefine transient domain")); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index fc9739e..9e1c6e3 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1701,7 +1701,9 @@ vboxDomainDestroyFlags(virDomainPtr dom, PRBool isAccessible = PR_FALSE; nsresult rc; - virCheckFlags(0, -1); + /* No transient domains, so these flags are trivially ignored. */ + virCheckFlags(VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL, -1); vboxIIDFromUUID(&iid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine); @@ -4982,10 +4984,15 @@ vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags) #if VBOX_API_VERSION >= 4000 vboxArray media = VBOX_ARRAY_INITIALIZER; #endif - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); + /* VBox snapshots maintain no metadata, so we can trivially ignore + * that flag. XXX Wire up VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL. */ + #if VBOX_API_VERSION < 4000 /* Block for checking if HDD's are attched to VM. * considering just IDE bus for now. Also skipped -- 1.7.4.4

Prepare for code sharing. No semantic change. * src/qemu/qemu_driver.c (qemuFindQemuImgBinary) (qemuDomainSnapshotWriteMetadata) (qemuDomainSnapshotDiscard): Float up. (qemuDomainSnapshotDiscardDescendant): Likewise, and rename... (qemuDomainSnapshotDiscardAll): ...for generic use. (qemuDomainSnapshotDelete): Update caller. --- src/qemu/qemu_driver.c | 371 ++++++++++++++++++++++++------------------------ 1 files changed, 188 insertions(+), 183 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6557854..90b1d91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1580,6 +1580,193 @@ cleanup: } +/* Locate an appropriate 'qemu-img' binary. */ +static char * +qemuFindQemuImgBinary(void) +{ + char *ret; + + ret = virFindFileInPath("kvm-img"); + if (ret == NULL) + ret = virFindFileInPath("qemu-img"); + if (ret == NULL) + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + + return ret; +} + +static int +qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, + virDomainSnapshotObjPtr snapshot, + char *snapshotDir) +{ + int fd = -1; + char *newxml = NULL; + int ret = -1; + char *snapDir = NULL; + char *snapFile = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(vm->def->uuid, uuidstr); + newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); + if (newxml == NULL) { + virReportOOMError(); + return -1; + } + + if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virFileMakePath(snapDir) < 0) { + virReportSystemError(errno, _("cannot create snapshot directory '%s'"), + snapDir); + goto cleanup; + } + + if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR); + if (fd < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create snapshot file '%s'"), snapFile); + goto cleanup; + } + if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { + virReportSystemError(errno, _("Failed to write snapshot data to %s"), + snapFile); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(snapFile); + VIR_FREE(snapDir); + VIR_FREE(newxml); + VIR_FORCE_CLOSE(fd); + return ret; +} + +/* Discard one snapshot (or its metadata), without reparenting any children. */ +static int +qemuDomainSnapshotDiscard(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool metadata_only) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; + char *snapFile = NULL; + int ret = -1; + int i; + qemuDomainObjPrivatePtr priv; + virDomainSnapshotObjPtr parentsnap; + + if (!metadata_only) { + if (!virDomainObjIsActive(vm)) { + 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 */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + /* we continue on even in the face of error, since other + * disks in this VM may have this snapshot in place + */ + continue; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) { + /* we continue on even in the face of error, since other + * disks in this VM may have this snapshot in place + */ + continue; + } + } + } + } else { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + } + + if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, + vm->def->name, snap->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (snap == vm->current_snapshot) { + if (snap->def->parent) { + parentsnap = virDomainSnapshotFindByName(&vm->snapshots, + snap->def->parent); + if (!parentsnap) { + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent); + } else { + parentsnap->def->current = true; + if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, + driver->snapshotDir) < 0) { + VIR_WARN("failed to set parent snapshot '%s' as current", + snap->def->parent); + parentsnap->def->current = false; + parentsnap = NULL; + } + } + } + vm->current_snapshot = parentsnap; + } + + if (unlink(snapFile) < 0) + VIR_WARN("Failed to unlink %s", snapFile); + virDomainSnapshotObjListRemove(&vm->snapshots, snap); + + ret = 0; + +cleanup: + VIR_FREE(snapFile); + VIR_FREE(qemuimgarg[0]); + + return ret; +} + +struct snap_remove { + struct qemud_driver *driver; + virDomainObjPtr vm; + bool metadata_only; + int err; +}; + +/* Hash iterator callback to discard multiple snapshots. */ +static void +qemuDomainSnapshotDiscardAll(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + struct snap_remove *curr = data; + int err; + + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, + curr->metadata_only); + if (err && !curr->err) + curr->err = err; +} + static int qemuDomainDestroyFlags(virDomainPtr dom, unsigned int flags) @@ -8283,75 +8470,6 @@ cleanup: return ret; } -static char *qemuFindQemuImgBinary(void) -{ - char *ret; - - ret = virFindFileInPath("kvm-img"); - if (ret == NULL) - ret = virFindFileInPath("qemu-img"); - if (ret == NULL) - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to find kvm-img or qemu-img")); - - return ret; -} - -static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, - virDomainSnapshotObjPtr snapshot, - char *snapshotDir) -{ - int fd = -1; - char *newxml = NULL; - int ret = -1; - char *snapDir = NULL; - char *snapFile = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(vm->def->uuid, uuidstr); - newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); - if (newxml == NULL) { - virReportOOMError(); - return -1; - } - - if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - if (virFileMakePath(snapDir) < 0) { - virReportSystemError(errno, _("cannot create snapshot directory '%s'"), - snapDir); - goto cleanup; - } - - if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR); - if (fd < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create snapshot file '%s'"), snapFile); - goto cleanup; - } - if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { - virReportSystemError(errno, _("Failed to write snapshot data to %s"), - snapFile); - goto cleanup; - } - - ret = 0; - -cleanup: - VIR_FREE(snapFile); - VIR_FREE(snapDir); - VIR_FREE(newxml); - VIR_FORCE_CLOSE(fd); - return ret; -} - - static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { int i; @@ -8974,119 +9092,6 @@ cleanup: return ret; } -static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap, - bool metadata_only) -{ - const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; - char *snapFile = NULL; - int ret = -1; - int i; - qemuDomainObjPrivatePtr priv; - virDomainSnapshotObjPtr parentsnap = NULL; - - if (!metadata_only) { - if (!virDomainObjIsActive(vm)) { - 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 */ - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { - /* we continue on even in the face of error, since other - * disks in this VM may have this snapshot in place - */ - continue; - } - - qemuimgarg[4] = vm->def->disks[i]->src; - - if (virRun(qemuimgarg, NULL) < 0) { - /* we continue on even in the face of error, since other - * disks in this VM may have this snapshot in place - */ - continue; - } - } - } - } else { - priv = vm->privateData; - qemuDomainObjEnterMonitorWithDriver(driver, vm); - /* we continue on even in the face of error */ - qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); - } - } - - if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, - vm->def->name, snap->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (snap == vm->current_snapshot) { - if (snap->def->parent) { - parentsnap = virDomainSnapshotFindByName(&vm->snapshots, - snap->def->parent); - if (!parentsnap) { - VIR_WARN("missing parent snapshot matching name '%s'", - snap->def->parent); - } else { - parentsnap->def->current = true; - if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, - driver->snapshotDir) < 0) { - VIR_WARN("failed to set parent snapshot '%s' as current", - snap->def->parent); - parentsnap->def->current = false; - parentsnap = NULL; - } - } - } - vm->current_snapshot = parentsnap; - } - - if (unlink(snapFile) < 0) - VIR_WARN("Failed to unlink %s", snapFile); - virDomainSnapshotObjListRemove(&vm->snapshots, snap); - - ret = 0; - -cleanup: - VIR_FREE(snapFile); - VIR_FREE(qemuimgarg[0]); - - return ret; -} - -struct snap_remove { - struct qemud_driver *driver; - virDomainObjPtr vm; - bool metadata_only; - int err; -}; - -static void -qemuDomainSnapshotDiscardDescendant(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) -{ - virDomainSnapshotObjPtr snap = payload; - struct snap_remove *curr = data; - int err; - - err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, - curr->metadata_only); - if (err && !curr->err) - curr->err = err; -} - struct snap_reparent { struct qemud_driver *driver; const char *parent; @@ -9164,7 +9169,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rem.err = 0; virDomainSnapshotForEachDescendant(&vm->snapshots, snap, - qemuDomainSnapshotDiscardDescendant, + qemuDomainSnapshotDiscardAll, &rem); if (rem.err < 0) goto endjob; -- 1.7.4.4

A nice benefit of deleting all snapshots at undefine time is that you don't have to do any reparenting or subtree identification - since everything goes, this is an O(n) process, whereas using multiple virDomainSnapshotDelete calls would be O(n^2) or worse. * src/qemu/qemu_driver.c (qemuDomainDestroyFlags) (qemuDomainUndefineFlags): Honor new flags. --- src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5a18309..f79d9bc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1778,7 +1778,8 @@ qemuDomainDestroyFlags(virDomainPtr dom, qemuDomainObjPrivatePtr priv; int nsnapshots; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1792,10 +1793,24 @@ qemuDomainDestroyFlags(virDomainPtr dom, if (!vm->persistent && (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("cannot delete transient domain with %d snapshots"), - nsnapshots); - goto cleanup; + struct snap_remove rem; + + if ((flags & (VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete transient domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = !(flags & VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL); + rem.err = 0; + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + if (rem.err < 0) + goto cleanup; } priv = vm->privateData; @@ -4919,7 +4934,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, int ret = -1; int nsnapshots; - virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4934,10 +4951,24 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("cannot delete inactive domain with %d snapshots"), - nsnapshots); - goto cleanup; + struct snap_remove rem; + + if ((flags & (VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = !(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL); + rem.err = 0; + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + if (rem.err < 0) + goto cleanup; } if (!vm->persistent) { -- 1.7.4.4

Similar to 'undefine --managed-save' (commit 83e849c1), we must assume that the old API is unsafe, and emulate it ourselves. Additionally, we have the wrinkle that while virDomainUndefineFlags and managed save cleanup were introduced in 0.9.4, it wasn't until 0.9.5 that snapshots block undefine of a domain. Simulate as much as possible with older servers, but the --snapshots-metadata support requires a server >= 0.9.5. Same story for virDomainDestroyFlags, and virDomainShutdownFlags doesn't exist yet. Oh well. * tools/virsh.c (cmdUndefine, cmdDestroy, cmdShutdown): Add --snapshots-full and --snapshots-metadata flags. (vshRemoveAllSnapshots): New helper method. * tools/virsh.pod (undefine, destroy, shutdown): Document them. --- tools/virsh.c | 400 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 36 +++++- 2 files changed, 386 insertions(+), 50 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 15b9bdd..8069a8c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1422,6 +1422,59 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) return ret; } +/* Helper for undefine, shutdown, and destroy */ +static int +vshRemoveAllSnapshots(virDomainPtr dom, int nsnapshots, bool full) +{ + int ret = -1; + char **names; + int actual; + int i; + virDomainSnapshotPtr snapshot = NULL; + int flags = 0; + + /* It's likely that if virDomainUndefineFlags was unsupported to + * get us here in the first place, then + * VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY will also be + * unsupported. But better to hear that from the driver. + */ + if (!full) + flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY; + + if (VIR_ALLOC_N(names, nsnapshots) < 0) + goto cleanup; + + actual = virDomainSnapshotListNames(dom, names, nsnapshots, 0); + if (actual < 0) + goto cleanup; + + /* Sadly, this is an O(n) loop over a function call that is also a + * minimum of O(n), for a complexity of O(n^2), whereas newer + * servers that support the delete in the undefine action are + * O(n). Oh well. */ + for (i = 0; i < actual; i++) { + if (snapshot) + virDomainSnapshotFree(snapshot); + + snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); + if (snapshot == NULL) + continue; + + if (virDomainSnapshotDelete(snapshot, flags) < 0) + goto cleanup; + } + + ret = 0; + +cleanup: + if (snapshot) + virDomainSnapshotFree(snapshot); + for (i = 0; i < actual; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + return ret; +} + /* * "undefine" command */ @@ -1434,6 +1487,10 @@ static const vshCmdInfo info_undefine[] = { static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, {"managed-save", VSH_OT_BOOL, 0, N_("remove domain managed state file")}, + {"snapshots-metadata", VSH_OT_BOOL, 0, + N_("remove all domain snapshot metadata, if inactive")}, + {"snapshots-full", VSH_OT_BOOL, 0, + N_("remove all domain snapshot contents, if inactive")}, {NULL, 0, 0, NULL} }; @@ -1441,18 +1498,36 @@ static bool cmdUndefine(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; const char *name = NULL; + /* Flags to attempt. */ int flags = 0; - int managed_save = vshCommandOptBool(cmd, "managed-save"); + /* User-requested actions. */ + bool managed_save = vshCommandOptBool(cmd, "managed-save"); + bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); + bool snapshots_full = vshCommandOptBool(cmd, "snapshots-full"); + /* Positive if these items exist. */ int has_managed_save = 0; + int has_snapshots_metadata = 0; + int has_snapshots = 0; + /* True if undefine will not strand data, even on older servers. */ + bool managed_save_safe = false; + bool snapshots_safe = false; int rc = -1; + int running; - if (managed_save) + if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; - - if (!managed_save) - flags = -1; + managed_save_safe = true; + } + if (snapshots_metadata) { + flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA; + snapshots_safe = true; + } + if (snapshots_full) { + flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL; + snapshots_safe = true; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1460,61 +1535,124 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - has_managed_save = virDomainHasManagedSaveImage(dom, 0); - if (has_managed_save < 0) { - if (last_error->code != VIR_ERR_NO_SUPPORT) { - virshReportError(ctl); - virDomainFree(dom); - return false; - } else { + /* Do some flag manipulation. The goal here is to disable bits + * from flags to reduce the likelihood of a server rejecting + * unknown flag bits, as well as to track conditions which are + * safe by default for the given hypervisor and server version. */ + running = virDomainIsActive(dom); + if (running < 0) { + virshReportError(ctl); + goto cleanup; + } + if (!running) { + /* Undefine with snapshots only fails for inactive domains, + * and managed save only exists on inactive domains; if + * running, then we don't want to remove anything. */ + has_managed_save = virDomainHasManagedSaveImage(dom, 0); + if (has_managed_save < 0) { + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virshReportError(ctl); + goto cleanup; + } virFreeError(last_error); last_error = NULL; - } - } - - if (flags == -1) { - if (has_managed_save == 1) { - vshError(ctl, - _("Refusing to undefine while domain managed save " - "image exists")); - virDomainFree(dom); - return false; + has_managed_save = 0; } - rc = virDomainUndefine(dom); - } else { - rc = virDomainUndefineFlags(dom, flags); - - /* It might fail when virDomainUndefineFlags is not - * supported on older libvirt, try to undefine the - * domain with combo virDomainManagedSaveRemove and - * virDomainUndefine. - */ - if (rc < 0) { + has_snapshots = virDomainSnapshotNum(dom, 0); + if (has_snapshots < 0) { if (last_error->code != VIR_ERR_NO_SUPPORT) { virshReportError(ctl); - goto end; - } else { + goto cleanup; + } + virFreeError(last_error); + last_error = NULL; + has_snapshots = 0; + } + if (has_snapshots) { + has_snapshots_metadata + = virDomainSnapshotNum(dom, VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (has_snapshots_metadata < 0) { + /* The server did not know the new flag, assume that all + snapshots have metadata. */ virFreeError(last_error); last_error = NULL; + has_snapshots_metadata = has_snapshots; + } else { + /* The server knew the new flag, all aspects of + * undefineFlags are safe. */ + managed_save_safe = snapshots_safe = true; } + } + } + if (!has_managed_save) { + flags &= ~VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; + managed_save_safe = true; + } + if (has_snapshots == 0) { + flags &= ~VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL; + snapshots_safe = true; + } + if (has_snapshots_metadata == 0) { + flags &= ~VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA; + snapshots_safe = true; + } - if ((has_managed_save == 1) && - virDomainManagedSaveRemove(dom, 0) < 0) - goto end; + /* Generally we want to try the new API first. However, while + * virDomainUndefineFlags was introduced at the same time as + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in 0.9.4, the + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA flag was not present + * until 0.9.5; skip to piecewise emulation if we couldn't prove + * above that the new API is safe. */ + if (managed_save_safe && snapshots_safe) { + rc = virDomainUndefineFlags(dom, flags); + if (rc == 0 || (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG)) + goto out; + virFreeError(last_error); + last_error = NULL; + } + + /* The new API is unsupported or unsafe; fall back to doing things + * piecewise. */ + if (has_managed_save) { + if (!managed_save) { + vshError(ctl, "%s", + _("Refusing to undefine while domain managed save " + "image exists")); + goto cleanup; + } + if (virDomainManagedSaveRemove(dom, 0) < 0) { + virshReportError(ctl); + goto cleanup; + } + } - rc = virDomainUndefine(dom); + if (has_snapshots) { + if (has_snapshots_metadata && + !(snapshots_metadata || snapshots_full)) { + vshError(ctl, + _("Refusing to undefine while %d snapshots exist"), + has_snapshots); + goto cleanup; } + + if ((snapshots_metadata || snapshots_full) && + vshRemoveAllSnapshots(dom, has_snapshots, snapshots_full) < 0) + goto cleanup; } -end: + rc = virDomainUndefine(dom); + +out: if (rc == 0) { vshPrint(ctl, _("Domain %s has been undefined\n"), name); + ret = true; } else { vshError(ctl, _("Failed to undefine domain %s"), name); - ret = false; } +cleanup: virDomainFree(dom); return ret; } @@ -2500,6 +2638,10 @@ static const vshCmdInfo info_shutdown[] = { static const vshCmdOptDef opts_shutdown[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"snapshots-metadata", VSH_OT_BOOL, 0, + N_("remove all domain snapshot metadata, if transient")}, + {"snapshots-full", VSH_OT_BOOL, 0, + N_("remove all domain snapshot contents, if transient")}, {NULL, 0, 0, NULL} }; @@ -2507,8 +2649,15 @@ static bool cmdShutdown(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; const char *name; + /* User-requested actions. */ + bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); + bool snapshots_full = vshCommandOptBool(cmd, "snapshots-full"); + /* Positive if these items exist. */ + int has_snapshots_metadata = 0; + int has_snapshots = 0; + int persistent; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -2516,13 +2665,67 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; + /* Do some flag manipulation. The goal here is to track + * conditions which are safe by default for the given hyperviser + * and server version. */ + persistent = virDomainIsPersistent(dom); + if (persistent < 0) { + virshReportError(ctl); + goto cleanup; + } + if (!persistent) { + /* Snapshot safety is only needed for transient domains. */ + has_snapshots = virDomainSnapshotNum(dom, 0); + if (has_snapshots < 0) { + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virshReportError(ctl); + goto cleanup; + } + virFreeError(last_error); + last_error = NULL; + has_snapshots = 0; + } + if (has_snapshots) { + has_snapshots_metadata + = virDomainSnapshotNum(dom, VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (has_snapshots_metadata < 0) { + /* The server did not know the new flag, assume that all + snapshots have metadata. */ + virFreeError(last_error); + last_error = NULL; + has_snapshots_metadata = has_snapshots; + } + } + } + + /* XXX Once virDomainShutdownFlags is added, use it here. Since + * snapshot shutdown safety was introduced before the new API, we + * don't have to go through quite as many contortions as + * cmdDestroy to check for snapshot safety. */ + + /* Fall back to doing things piecewise. */ + if (has_snapshots) { + if (has_snapshots_metadata && + !(snapshots_metadata || snapshots_full)) { + vshError(ctl, + _("Refusing to shutdown while %d snapshots exist"), + has_snapshots); + goto cleanup; + } + + if ((snapshots_metadata || snapshots_full) && + vshRemoveAllSnapshots(dom, has_snapshots, snapshots_full) < 0) + goto cleanup; + } + if (virDomainShutdown(dom) == 0) { vshPrint(ctl, _("Domain %s is being shutdown\n"), name); + ret = true; } else { vshError(ctl, _("Failed to shutdown domain %s"), name); - ret = false; } +cleanup: virDomainFree(dom); return ret; } @@ -2577,6 +2780,10 @@ static const vshCmdInfo info_destroy[] = { static const vshCmdOptDef opts_destroy[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"snapshots-metadata", VSH_OT_BOOL, 0, + N_("remove all domain snapshot metadata, if transient")}, + {"snapshots-full", VSH_OT_BOOL, 0, + N_("remove all domain snapshot contents, if transient")}, {NULL, 0, 0, NULL} }; @@ -2584,8 +2791,29 @@ static bool cmdDestroy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; const char *name; + /* Flags to attempt. */ + int flags = 0; + /* User-requested actions. */ + bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); + bool snapshots_full = vshCommandOptBool(cmd, "snapshots-full"); + /* Positive if these items exist. */ + int has_snapshots_metadata = 0; + int has_snapshots = 0; + /* True if undefine will not strand data, even on older servers. */ + bool snapshots_safe = false; + int rc = -1; + int persistent; + + if (snapshots_metadata) { + flags |= VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA; + snapshots_safe = true; + } + if (snapshots_full) { + flags |= VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL; + snapshots_safe = true; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -2593,13 +2821,93 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainDestroy(dom) == 0) { + /* Do some flag manipulation. The goal here is to disable bits + * from flags to reduce the likelihood of a server rejecting + * unknown flag bits, as well as to track conditions which are + * safe by default for the given hypervisor and server version. */ + persistent = virDomainIsPersistent(dom); + if (persistent < 0) { + virshReportError(ctl); + goto cleanup; + } + if (!persistent) { + /* Snapshot safety is only needed for transient domains. */ + has_snapshots = virDomainSnapshotNum(dom, 0); + if (has_snapshots < 0) { + if (last_error->code != VIR_ERR_NO_SUPPORT) { + virshReportError(ctl); + goto cleanup; + } + virFreeError(last_error); + last_error = NULL; + has_snapshots = 0; + } + if (has_snapshots) { + has_snapshots_metadata + = virDomainSnapshotNum(dom, VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (has_snapshots_metadata < 0) { + /* The server did not know the new flag, assume that all + snapshots have metadata. */ + virFreeError(last_error); + last_error = NULL; + has_snapshots_metadata = has_snapshots; + } else { + /* The server knew the new flag, all aspects of + * destroyFlags are safe. */ + snapshots_safe = true; + } + } + } + if (has_snapshots == 0) { + flags &= ~VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL; + snapshots_safe = true; + } + if (has_snapshots_metadata == 0) { + flags &= ~VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA; + snapshots_safe = true; + } + + /* Generally we want to try the new API first. However, while + * virDomainDestroyFlags was introduced in 0.9.4, the + * VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA flag was not present + * until 0.9.5; skip to piecewise emulation if we couldn't prove + * above that the new API is safe. */ + if (snapshots_safe) { + rc = virDomainDestroyFlags(dom, flags); + if (rc == 0 || (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG)) + goto out; + virFreeError(last_error); + last_error = NULL; + } + + /* The new API is unsupported or unsafe; fall back to doing things + * piecewise. */ + if (has_snapshots) { + if (has_snapshots_metadata && + !(snapshots_metadata || snapshots_full)) { + vshError(ctl, + _("Refusing to destroy while %d snapshots exist"), + has_snapshots); + goto cleanup; + } + + if ((snapshots_metadata || snapshots_full) && + vshRemoveAllSnapshots(dom, has_snapshots, snapshots_full) < 0) + goto cleanup; + } + + rc = virDomainDestroy(dom); + +out: + if (rc == 0) { vshPrint(ctl, _("Domain %s destroyed\n"), name); + ret = true; } else { vshError(ctl, _("Failed to destroy domain %s"), name); - ret = false; } +cleanup: virDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index e17f309..337fcf4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -409,7 +409,7 @@ B<Example> Define a domain from an XML <file>. The domain definition is registered but not started. -=item B<destroy> I<domain-id> +=item B<destroy> I<domain-id> [I<--snapshots-full>] [I<--snapshots-metadata>] Immediately terminate the domain domain-id. This doesn't give the domain OS any chance to react, and it's the equivalent of ripping the power @@ -418,6 +418,15 @@ the B<shutdown> command instead. However, this does not delete any storage volumes used by the guest, and if the domain is persistent, it can be restarted later. +The I<--snapshots-full> and I<--snapshots-metadata> flags guarantee +that any snapshots (see the B<snapshot-list> command) are also cleaned +up when stopping a transient domain. Without either flag, attempts +to destroy a transient domain with snapshot metadata will fail. The +I<--snapshots-full> flag works with older servers, but loses all +snapshot contents; the I<--snapshots-metadata> flag only removes +metadata that can later be recreated, but requires newer servers. If +the domain is persistent, these flags are ignored. + =item B<domblkstat> I<domain> I<block-device> Get device block stats for a running domain. @@ -877,7 +886,7 @@ The I<--maximum> flag controls the maximum number of virtual cpus that can be hot-plugged the next time the domain is booted. As such, it must only be used with the I<--config> flag, and not with the I<--live> flag. -=item B<shutdown> I<domain-id> +=item B<shutdown> I<domain-id> [I<--snapshots-full>] [I<--snapshots-metadata>] Gracefully shuts down a domain. This coordinates with the domain OS to perform graceful shutdown, so there is no guarantee that it will @@ -887,6 +896,15 @@ services must be shutdown in the domain. The exact behavior of a domain when it shuts down is set by the I<on_shutdown> parameter in the domain's XML definition. +The I<--snapshots-full> and I<--snapshots-metadata> flags guarantee +that any snapshots (see the B<snapshot-list> command) are also cleaned +up when shutting down a transient domain. Without either flag, attempts +to shutdown a transient domain with snapshot metadata will fail. The +I<--snapshots-full> flag works with older servers, but loses all +snapshot contents; the I<--snapshots-metadata> flag only removes +metadata that can later be recreated, but requires newer servers. If +the domain is persistent, these flags are ignored. + =item B<start> I<domain-name> [I<--console>] [I<--paused>] [I<--autodestroy>] [I<--bypass-cache>] @@ -917,16 +935,26 @@ hypervisor. Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1. -=item B<undefine> I<domain-id> [I<--managed-save>] +=item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-full>] +[I<--snapshots-metadata] Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, the domain configuration is removed. -The I<--managed-save> flag guarantees that any managed save image(see +The I<--managed-save> flag guarantees that any managed save image (see the B<managedsave> command) is also cleaned up. Without the flag, attempts to undefine a domain with a managed save image will fail. +The I<--snapshots-full> and I<--snapshots-metadata> flags guarantee +that any snapshots (see the B<snapshot-list> command) are also cleaned +up when undefining an inactive domain. Without either flag, attempts +to undefine an inactive domain with snapshot metadata will fail. The +I<--snapshots-full> flag works with older servers, but loses all +snapshot contents; the I<--snapshots-metadata> flag only removes +metadata that can later be recreated, but requires newer servers. If +the domain is active, these flags are ignored. + NOTE: For an inactive domain, the domain name or UUID must be used as the I<domain-id>. -- 1.7.4.4

Just like VM saved state images (virsh save), snapshots MUST track the inactive domain xml to detect any ABI incompatibilities. The indentation is not perfect, but functionality comes before form. Later patches will actually supply a full domain; for now, this wires up the storage to support one, but doesn't ever generate one in dumpxml output. Happily, libvirt.c was already rejecting use of VIR_DOMAIN_XML_SECURE from read-only connections, even though before this patch, there was no one ever using that flag and there was no information to be secured by the use of that flag. * src/libvirt.c (virDomainSnapshotGetXMLDesc): Document flag. * src/conf/domain_conf.h (_virDomainSnapshotDef): Add member. (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Update signature. * src/conf/domain_conf.c (virDomainSnapshotDefFree): Clean up. (virDomainSnapshotDefParseString): Optionally parse domain. (virDomainSnapshotDefFormat): Output full domain. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML) (esxDomainSnapshotGetXMLDesc): Update callers. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML) (vboxDomainSnapshotGetXMLDesc): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotLoad, qemuDomainSnapshotGetXMLDesc) (qemuDomainSnapshotWriteMetadata): Likewise. * docs/formatsnapshot.html.in: Rework doc example. Based on a patch by Philipp Hahn. --- docs/formatsnapshot.html.in | 47 ++++++++++++++++++++++++++++------------ src/conf/domain_conf.c | 50 +++++++++++++++++++++++++++++++++++++------ src/conf/domain_conf.h | 7 +++++- src/esx/esx_driver.c | 4 +- src/libvirt.c | 7 +++++- src/qemu/qemu_driver.c | 20 +++++++++++++---- src/vbox/vbox_tmpl.c | 4 +- 7 files changed, 107 insertions(+), 32 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 79ed1d2..edf20e8 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -46,27 +46,46 @@ snapshots. Readonly. </dd> <dt><code>domain</code></dt> - <dd>The domain that this snapshot was taken against. This - element contains exactly one child element, uuid. This - specifies the uuid of the domain that this snapshot was taken - against. Readonly. + <dd>The domain that this snapshot was taken against. Older + versions of libvirt stored only a single child element, uuid; + reverting to a snapshot like this is risky if the current + state of the domain differs from the state that the domain was + created in, and requires the use of the + <code>VIR_DOMAIN_SNAPSHOT_REVERT_FORCE</code> flag + in <code>virDomainRevertToSnapshot()</code>. Newer versions + of libvirt store the entire + inactive <a href="formatdomain.html">domain configuration</a> + at the time of the snapshot. Readonly. </dd> </dl> - <h2><a name="example">Example</a></h2> + <h2><a name="example">Examples</a></h2> + <p>Using this XML on creation:</p> <pre> &lt;domainsnapshot&gt; - &lt;name&gt;os-updates&lt;/name&gt; &lt;description&gt;Snapshot of OS install and updates&lt;/description&gt; - &lt;state&gt;running&lt;/state&gt; - &lt;creationTime&gt;1270477159&lt;/creationTime&gt; - &lt;parent&gt; - &lt;name&gt;bare-os-install&lt;/name&gt; - &lt;/parent&gt; - &lt;domain&gt; - &lt;uuid&gt;93a5c045-6457-2c09-e56c-927cdf34e178&lt;/uuid&gt; - &lt;/domain&gt; &lt;/domainsnapshot&gt;</pre> + + <p>Will result in XML similar to this from + virDomainSnapshotGetXMLDesc:</p> + <pre> +&lt;domainsnapshot&gt; + &lt;name&gt;1270477159&lt;/name&gt; + &lt;description&gt;Snapshot of OS install and updates&lt;/description&gt; + &lt;state&gt;running&lt;/state&gt; + &lt;creationTime&gt;1270477159&lt;/creationTime&gt; + &lt;parent&gt; + &lt;name&gt;bare-os-install&lt;/name&gt; + &lt;/parent&gt; + &lt;domain&gt; + &lt;name&gt;fedora&lt;/name&gt; + &lt;uuid&gt;93a5c045-6457-2c09-e56c-927cdf34e178&lt;/uuid&gt; + &lt;memory&gt;1048576&lt;/memory&gt; + ... + &lt;/devices&gt; + &lt;/domain&gt; +&lt;/domainsnapshot&gt;</pre> + </body> </html> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27c77fe..d2a9849 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10953,11 +10953,17 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + virDomainDefFree(def->dom); VIR_FREE(def); } -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) +/* If newSnapshot is true, caps, expectedVirtTypes, and flags are ignored. */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -10966,6 +10972,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, char *creation = NULL, *state = NULL; struct timeval tv; int active; + char *tmp; xml = virXMLParseCtxt(NULL, xmlStr, "domainsnapshot.xml", &ctxt); if (!xml) { @@ -11028,9 +11035,29 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, goto cleanup; } def->current = active != 0; - } - else + + /* Older snapshots were created with just <domain>/<uuid>, and + * lack domain/@type. In that case, leave dom NULL, and + * clients will have to decide between best effort + * initialization or outright failure. */ + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + VIR_FREE(tmp); + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + if (!domainNode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + def->dom = virDomainDefParseNode(caps, xml, domainNode, + expectedVirtTypes, flags); + if (!def->dom) + goto cleanup; + } else { + VIR_WARN("parsing older snapshot that lacks domain"); + } + } else { def->creationTime = tv.tv_sec; + } ret = def; @@ -11047,10 +11074,15 @@ cleanup: char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal) { virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + flags |= VIR_DOMAIN_XML_INACTIVE; + virBufferAddLit(&buf, "<domainsnapshot>\n"); virBufferAsprintf(&buf, " <name>%s</name>\n", def->name); if (def->description) @@ -11065,9 +11097,13 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, " <domain>\n"); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, " </domain>\n"); + if (def->dom) { + virDomainDefFormatInternal(def->dom, flags, &buf); + } else { + virBufferAddLit(&buf, " <domain>\n"); + virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); + virBufferAddLit(&buf, " </domain>\n"); + } if (internal) virBufferAsprintf(&buf, " <active>%d</active>\n", def->current); virBufferAddLit(&buf, "</domainsnapshot>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 503fb58..93bb7c8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1302,6 +1302,7 @@ struct _virDomainSnapshotDef { char *parent; long long creationTime; /* in seconds */ int state; + virDomainDefPtr dom; /* Internal use. */ bool current; /* At most one snapshot in the list should have this set */ @@ -1325,10 +1326,14 @@ struct _virDomainSnapshotObjList { }; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal); virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 90f55c3..8fbbe8a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4223,7 +4223,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0); if (def == NULL) { return NULL; @@ -4319,7 +4319,7 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(snapshot->domain->uuid, uuid_string); - xml = virDomainSnapshotDefFormat(uuid_string, &def, 0); + xml = virDomainSnapshotDefFormat(uuid_string, &def, flags, 0); cleanup: esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotList); diff --git a/src/libvirt.c b/src/libvirt.c index b0160fb..fac2047 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15517,10 +15517,15 @@ error: /** * virDomainSnapshotGetXMLDesc: * @snapshot: a domain snapshot object - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-OR of subset of virDomainXMLFlags * * Provide an XML description of the domain snapshot. * + * No security-sensitive data will be included unless @flags contains + * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only + * connections. For this API, @flags should not contain either + * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8d769c9..7cf945a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -337,7 +337,10 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, 0, qemu_driver->caps, + QEMU_EXPECTED_VIRT_TYPES, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -1610,7 +1613,8 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); - newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); + newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, + VIR_DOMAIN_XML_SECURE, 1); if (newxml == NULL) { virReportOOMError(); return -1; @@ -1636,6 +1640,12 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, _("failed to create snapshot file '%s'"), snapFile); goto cleanup; } + /* XXX need virsh snapshot-edit, before this makes sense: + * char *tmp; + * virAsprintf(&tmp, "snapshot-edit %s", vm->def->name); + * virEmitXMLWarning(fd, snapshot->def->name, tmp); + * VIR_FREE(tmp); + */ if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { virReportSystemError(errno, _("Failed to write snapshot data to %s"), snapFile); @@ -8681,7 +8691,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) @@ -8908,7 +8918,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virCheckFlags(0, NULL); + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); @@ -8927,7 +8937,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; } - xml = virDomainSnapshotDefFormat(uuidstr, snap->def, 0); + xml = virDomainSnapshotDefFormat(uuidstr, snap->def, flags, 0); cleanup: if (vm) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9e1c6e3..0dd5efa 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5661,7 +5661,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(0, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); @@ -5843,7 +5843,7 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, def->state = VIR_DOMAIN_SHUTOFF; virUUIDFormat(dom->uuid, uuidstr); - ret = virDomainSnapshotDefFormat(uuidstr, def, 0); + ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0); cleanup: virDomainSnapshotDefFree(def); -- 1.7.4.4

* docs/schemas/domain.rng: Move guts... * docs/schemas/domaincommon.rng: ...to new file. * docs/schemas/domainsnapshot.rng: Allow new xml. * docs/schemas/Makefile.am (schema_DATA): Distribute new file. * tests/domainsnapshotxml2xmlout/full_domain.xml: New test. --- I've compressed this email by eliding the code motion; see git for the full patch. docs/schemas/Makefile.am | 1 + docs/schemas/domain.rng | 2555 +----------------------- docs/schemas/{domain.rng => domaincommon.rng} | 8 +- docs/schemas/domainsnapshot.rng | 14 +- tests/domainsnapshotxml2xmlout/full_domain.xml | 35 + 5 files changed, 49 insertions(+), 2564 deletions(-) copy docs/schemas/{domain.rng => domaincommon.rng} (99%) create mode 100644 tests/domainsnapshotxml2xmlout/full_domain.xml diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 596c207..4413d9e 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -6,6 +6,7 @@ schema_DATA = \ basictypes.rng \ capability.rng \ domain.rng \ + domaincommon.rng \ domainsnapshot.rng \ interface.rng \ network.rng \ diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index dd8c41a..cf0be68 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -5,2558 +5,5 @@ <ref name="domain"/> </start> - <include href='basictypes.rng'/> - <include href='storageencryption.rng'/> - <include href='networkcommon.rng'/> - - <!-- - description element, maybe placed anywhere under the root ... - <define name="filter-param-value"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.:]+</param> - </data> - </define> + <include href='domaincommon.rng'/> </grammar> diff --git a/docs/schemas/domain.rng b/docs/schemas/domaincommon.rng similarity index 99% copy from docs/schemas/domain.rng copy to docs/schemas/domaincommon.rng index dd8c41a..756e892 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domaincommon.rng @@ -1,16 +1,12 @@ <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> - <!-- We handle only document defining a domain --> - <start> - <ref name="domain"/> - </start> - + <!-- domain-related definitions used in multiple grammars --> <include href='basictypes.rng'/> <include href='storageencryption.rng'/> <include href='networkcommon.rng'/> <!-- - description element, maybe placed anywhere under the root + description element, may be placed anywhere under the root --> <define name="description"> <element name="description"> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 410833f..a16d731 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -1,9 +1,12 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for the libvirt domain snapshot properties XML format --> <grammar xmlns="http://relaxng.org/ns/structure/1.0"> <start> <ref name='domainsnapshot'/> </start> + <include href='domaincommon.rng'/> + <define name='domainsnapshot'> <element name='domainsnapshot'> <interleave> @@ -36,11 +39,14 @@ </element> </optional> <optional> - <element name='domain'> - <element name='uuid'> - <text/> + <choice> + <element name='domain'> + <element name='uuid'> + <ref name="UUID"/> + </element> </element> - </element> + <ref name='domain'/> + </choice> </optional> <optional> <element name='parent'> diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/domainsnapshotxml2xmlout/full_domain.xml new file mode 100644 index 0000000..942bd7f --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/full_domain.xml @@ -0,0 +1,35 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <parent> + <name>earlier_snap</name> + </parent> + <state>running</state> + <creationTime>1272917631</creationTime> +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> + <active>1</active> +</domainsnapshot> -- 1.7.4.4

As discussed here: https://www.redhat.com/archives/libvir-list/2011-August/msg00361.html https://www.redhat.com/archives/libvir-list/2011-August/msg00552.html Adds snapshot attribute and transient sub-element: <devices> <disk type=... snapshot='no|internal|external'> ... <transient/> </disk> </devices> * docs/schemas/domaincommon.rng (snapshot): New define. (disk): Add snapshot and persistent attributes. * docs/formatdomain.html.in: Document them. * src/conf/domain_conf.h (virDomainDiskSnapshot): New enum. (_virDomainDiskDef): New fields. * tests/qemuxml2argvdata/qemuxml2argv-disk-transient.xml: New test of rng, no args counterpart until qemu support is complete. * tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.args: New file, snapshot attribute does not affect args. * tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.xml: Likewise. * tests/qemuxml2argvtest.c (mymain): Run new test. --- docs/formatdomain.html.in | 40 ++++++++++++++++++-- docs/schemas/domaincommon.rng | 17 ++++++++ src/conf/domain_conf.c | 35 ++++++++++++++++- src/conf/domain_conf.h | 12 ++++++ src/libvirt_private.syms | 2 + .../qemuxml2argv-disk-snapshot.args | 7 +++ .../qemuxml2argv-disk-snapshot.xml | 39 +++++++++++++++++++ .../qemuxml2argv-disk-transient.xml | 27 +++++++++++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-transient.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f46771d..911dee5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -889,7 +889,7 @@ <pre> ... &lt;devices&gt; - &lt;disk type='file'&gt; + &lt;disk type='file' snapshot='external'&gt; &lt;driver name="tap" type="aio" cache="default"/&gt; &lt;source file='/var/lib/xen/images/fv0'/&gt; &lt;target dev='hda' bus='ide'/&gt; @@ -910,8 +910,14 @@ &lt;/source&gt; &lt;target dev="hdb" bus="ide"/&gt; &lt;boot order='1'/&gt; + &lt;transient/&gt; &lt;address type='drive' controller='0' bus='1' unit='0'/&gt; &lt;/disk&gt; + &lt;disk type='block' device='cdrom'&gt; + &lt;driver name='qemu' type='raw'/&gt; + &lt;target def='hdc' bus='ide'/&gt; + &lt;readonly/&gt; + &lt;/disk&gt; &lt;/devices&gt; ...</pre> @@ -923,9 +929,23 @@ and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". - <span class="since">Since 0.0.3; "device" attribute since 0.1.4; - "network" attribute since 0.8.7</span></dd> + and "cdrom", defaulting to "disk". The + optional <code>snapshot</code> attribute indicates the default + behavior of the disk during disk snapshots: "internal" + requires a file format such as qcow2 that can store both the + snapshot and the data changes since the snapshot; + "external" will separate the snapshot from the live data; and + "no" means the disk will not participate in snapshots. + Read-only disks default to "no", while the default for other + disks depends on the hypervisor's capabilities. Some + hypervisors allow a per-snapshot choice as well, + during <a href="formatsnapshot.html">domain snapshot + creation</a>. Not all snapshot modes are supported; + for example, <code>snapshot='yes'</code> with a transient disk + generally does not make sense. <span class="since">Since 0.0.3; + "device" attribute since 0.1.4; + "network" attribute since 0.8.7; "snapshot" since + 0.9.5</span></dd> <dt><code>source</code></dt> <dd>If the disk <code>type</code> is "file", then the <code>file</code> attribute specifies the fully-qualified @@ -1032,11 +1052,23 @@ the <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> + <dt><code>readonly</code></dt> + <dd>If present, this indicates the device cannot be modified by + the guest. For now, this is the default for disks with + attribute <code>type='cdrom'</code>. + </dd> <dt><code>shareable</code></dt> <dd>If present, this indicates the device is expected to be shared between domains (assuming the hypervisor and OS support this), which means that caching should be deactivated for that device. </dd> + <dt><code>transient</code></dt> + <dd>If present, this indicates that changes to the device + contents should be reverted automatically when the guest + exits. With some hypervisors, marking a disk transient + prevents the domain from participating in migration or + snapshots. <span class="since">Since 0.9.5</span> + </dd> <dt><code>serial</code></dt> <dd>If present, this specify serial number of virtual hard drive. For example, it may look diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 756e892..0af9e0f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -617,6 +617,11 @@ </element> </optional> <optional> + <element name="transient"> + <empty/> + </element> + </optional> + <optional> <element name="serial"> <ref name="diskSerial"/> </element> @@ -628,6 +633,15 @@ <ref name="address"/> </optional> </define> + <define name="snapshot"> + <attribute name="snapshot"> + <choice> + <value>no</value> + <value>internal</value> + <value>external</value> + </choice> + </attribute> + </define> <define name="lease"> <element name="lease"> @@ -667,6 +681,9 @@ </choice> </attribute> </optional> + <optional> + <ref name="snapshot"/> + </optional> <choice> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7e8da15..df0e921 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -187,6 +187,12 @@ VIR_ENUM_IMPL(virDomainVirtioEventIdx, VIR_DOMAIN_VIRTIO_EVENT_IDX_LAST, "on", "off") +VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, + "default", + "no", + "internal", + "external") + VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, "ide", "fdc", @@ -2073,6 +2079,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr cur, host; char *type = NULL; char *device = NULL; + char *snapshot = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -2106,6 +2113,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->type = VIR_DOMAIN_DISK_TYPE_FILE; } + snapshot = virXMLPropString(node, "snapshot"); + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -2207,6 +2216,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) { + def->transient = 1; } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ @@ -2278,6 +2289,18 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } + if (snapshot) { + def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + if (def->snapshot <= 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot setting '%s'"), + snapshot); + goto error; + } + } else if (def->readonly) { + def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO; + } + if (bus) { if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2423,6 +2446,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, cleanup: VIR_FREE(bus); VIR_FREE(type); + VIR_FREE(snapshot); VIR_FREE(target); VIR_FREE(source); while (nhosts > 0) { @@ -2448,7 +2472,7 @@ cleanup: no_memory: virReportOOMError(); - error: +error: virDomainDiskDefFree(def); def = NULL; goto cleanup; @@ -8636,8 +8660,13 @@ virDomainDiskDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, - " <disk type='%s' device='%s'>\n", + " <disk type='%s' device='%s'", type, device); + if (def->snapshot && + !(def->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO && def->readonly)) + virBufferAsprintf(buf, " snapshot='%s'", + virDomainDiskSnapshotTypeToString(def->snapshot)); + virBufferAddLit(buf, ">\n"); if (def->driverName || def->driverType || def->cachemode || def->ioeventfd || def->event_idx) { @@ -8711,6 +8740,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <readonly/>\n"); if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (def->transient) + virBufferAddLit(buf, " <transient/>\n"); if (def->serial) virBufferEscapeString(buf, " <serial>%s</serial>\n", def->serial); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 93bb7c8..7dbf353 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -217,6 +217,15 @@ enum virDomainVirtioEventIdx { VIR_DOMAIN_VIRTIO_EVENT_IDX_LAST }; +enum virDomainDiskSnapshot { + VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, + VIR_DOMAIN_DISK_SNAPSHOT_NO, + VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, + VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + + VIR_DOMAIN_DISK_SNAPSHOT_LAST +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -238,8 +247,10 @@ struct _virDomainDiskDef { int iomode; int ioeventfd; int event_idx; + int snapshot; /* enum virDomainDiskSnapshot */ unsigned int readonly : 1; unsigned int shared : 1; + unsigned int transient : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; }; @@ -1689,6 +1700,7 @@ VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) VIR_ENUM_DECL(virDomainDiskIo) +VIR_ENUM_DECL(virDomainDiskSnapshot) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) VIR_ENUM_DECL(virDomainController) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 74c5ff4..9530567 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -286,6 +286,8 @@ virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; +virDomainDiskSnapshotTypeFromString; +virDomainDiskSnapshotTypeToString; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainFSDefFree; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.args new file mode 100644 index 0000000..7e62942 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi -boot c \ +-drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0,format=qcow2,cache=none \ +-drive file=/dev/HostVG/QEMUGuest3,if=ide,bus=2,unit=0,format=qcow2,cache=none \ +-drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,bus=1,unit=0,format=raw \ +-net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.xml new file mode 100644 index 0000000..aeb2315 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-snapshot.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk' snapshot='internal'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <disk type='block' device='cdrom' snapshot='no'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <disk type='block' device='disk' snapshot='external'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest3'/> + <target dev='hdb' bus='ide'/> + <address type='drive' controller='0' bus='2' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-transient.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-transient.xml new file mode 100644 index 0000000..df49c48 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-transient.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <transient/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e8da5e..c7b1707 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -359,6 +359,8 @@ mymain(void) DO_TEST("disk-ioeventfd", false, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_IOEVENTFD, QEMU_CAPS_VIRTIO_TX_ALG, QEMU_CAPS_DEVICE); + DO_TEST("disk-snapshot", false, + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("event_idx", false, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX, -- 1.7.4.4

New flag bits are worth exposing via virsh. Additionally, even though I recently added 'virsh snapshot-parent', doing it one snapshot at a time is painful, so make it possible to expand the snapshot-list table at once. In the case of snapshot-list --roots, it's possible to emulate this even when talking to an older server that lacks the bit; whereas --metadata requires a newer server. * tools/virsh.c (cmdSnapshotDumpXML, cmdSnapshotCurrent): Add --security-info. (cmdSnapshotList): Add --parent, --roots, --metadata. * tools/virsh.pod (snapshot-dumpxml, snapshot-current) (snapshot-list): Document these. --- tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 22 +++++++++++--- 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 8069a8c..db6c71e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12303,6 +12303,8 @@ static const vshCmdInfo info_snapshot_current[] = { static const vshCmdOptDef opts_snapshot_current[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"name", VSH_OT_BOOL, 0, N_("list the name, rather than the full xml")}, + {"security-info", VSH_OT_BOOL, 0, + N_("include security sensitive information in XML dump")}, {NULL, 0, 0, NULL} }; @@ -12314,6 +12316,10 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) int current; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12331,7 +12337,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (!(snapshot = virDomainSnapshotCurrent(dom, 0))) goto cleanup; - xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + xml = virDomainSnapshotGetXMLDesc(snapshot, flags); if (!xml) goto cleanup; @@ -12377,6 +12383,10 @@ static const vshCmdInfo info_snapshot_list[] = { static const vshCmdOptDef opts_snapshot_list[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"parent", VSH_OT_BOOL, 0, N_("add a column showing parent snapshot")}, + {"roots", VSH_OT_BOOL, 0, N_("list only snapshots without parents")}, + {"metadata", VSH_OT_BOOL, 0, + N_("list only snapshots that have metadata that would prevent undefine")}, {NULL, 0, 0, NULL} }; @@ -12385,6 +12395,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; + int flags = 0; + int parent_filter = 0; /* -1 for roots filtering, 0 for no parent + information needed, 1 for parent column */ int numsnaps; char **names = NULL; int actual = 0; @@ -12394,11 +12407,27 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char *doc = NULL; virDomainSnapshotPtr snapshot = NULL; char *state = NULL; + char *parent = NULL; long long creation_longlong; time_t creation_time_t; char timestr[100]; struct tm time_info; + if (vshCommandOptBool(cmd, "parent")) { + if (vshCommandOptBool(cmd, "roots")) { + vshError(ctl, "%s", + _("--parent and --roots are mutually exlusive")); + return false; + } + parent_filter = 1; + } else if (vshCommandOptBool(cmd, "roots")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + } + + if (vshCommandOptBool(cmd, "metadata")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; + } + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12406,19 +12435,35 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - numsnaps = virDomainSnapshotNum(dom, 0); + numsnaps = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + parent_filter = -1; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + numsnaps = virDomainSnapshotNum(dom, flags); + } if (numsnaps < 0) goto cleanup; - vshPrintExtra(ctl, " %-20s %-25s %s\n", _("Name"), _("Creation Time"), _("State")); - vshPrintExtra(ctl, "---------------------------------------------------\n"); + if (parent_filter > 0) + vshPrintExtra(ctl, " %-20s %-25s %-15s %s", + _("Name"), _("Creation Time"), _("State"), _("Parent")); + else + vshPrintExtra(ctl, " %-20s %-25s %s", + _("Name"), _("Creation Time"), _("State")); + vshPrintExtra(ctl, "\n\ +------------------------------------------------------------\n"); if (numsnaps) { if (VIR_ALLOC_N(names, numsnaps) < 0) goto cleanup; - actual = virDomainSnapshotListNames(dom, names, numsnaps, 0); + actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); if (actual < 0) goto cleanup; @@ -12426,6 +12471,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < actual; i++) { /* free up memory from previous iterations of the loop */ + VIR_FREE(parent); VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); @@ -12445,6 +12491,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!xml) continue; + if (parent_filter) { + parent = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); + if (!parent && parent_filter < 0) + continue; + } + state = virXPathString("string(/domainsnapshot/state)", ctxt); if (state == NULL) continue; @@ -12457,9 +12510,14 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) continue; } localtime_r(&creation_time_t, &time_info); - strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", + &time_info); - vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); + if (parent) + vshPrint(ctl, " %-20s %-25s %-15s %s\n", + names[i], timestr, state, parent); + else + vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); } } @@ -12467,6 +12525,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) cleanup: /* this frees up memory from the last iteration of the loop */ + VIR_FREE(parent); VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); @@ -12494,6 +12553,8 @@ static const vshCmdInfo info_snapshot_dumpxml[] = { static const vshCmdOptDef opts_snapshot_dumpxml[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"security-info", VSH_OT_BOOL, 0, + N_("include security sensitive information in XML dump")}, {NULL, 0, 0, NULL} }; @@ -12505,6 +12566,10 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12520,7 +12585,7 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) if (snapshot == NULL) goto cleanup; - xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + xml = virDomainSnapshotGetXMLDesc(snapshot, flags); if (!xml) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 337fcf4..0b319ee 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -527,7 +527,7 @@ Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be used. I<--inactive> tells virsh to dump domain configuration that will be used on next start of the domain as opposed to the current domain configuration. -Using I<--security-info> security sensitive information will also be included +Using I<--security-info> will also include security sensitive information in the XML dump. I<--update-cpu> updates domain CPU requirements according to host CPU. @@ -1607,19 +1607,31 @@ Create a snapshot for domain I<domain> with the given <name> and value. If I<--print-xml> is specified, then XML appropriate for I<snapshot-create> is output, rather than actually creating a snapshot. -=item B<snapshot-current> I<domain> [I<--name>] +=item B<snapshot-current> I<domain> [I<--name>] [I<--security-info>] Output the snapshot XML for the domain's current snapshot (if any). If I<--name> is specified, just list the snapshot name instead of the -full xml. +full xml. Otherwise, using I<--security-info> will also include security +sensitive information. -=item B<snapshot-list> I<domain> +=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots>}] [I<--metadata>] List all of the available snapshots for the given domain. -=item B<snapshot-dumpxml> I<domain> I<snapshot> +If I<--parent> is specified, add a column to the output table giving +the name of the parent of each snapshot. + +If I<--roots> is specified, the list will be filtered to just snapshots +that have no parents; this option is not compatible with I<--parent>. + +If I<--metadata> is specified, the list will be filtered to just +snapshots that involve libvirt metadata, and thus would interfere in +using the B<undefine> or B<destroy> commands on that domain. + +=item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] Output the snapshot XML for the domain's snapshot named I<snapshot>. +Using I<--security-info> will also include security sensitive information. =item B<snapshot-parent> I<domain> I<snapshot> -- 1.7.4.4

It would technically be possible to have virsh compute the list of descendants of a given snapshot, then delete those one at a time. But it's complex, and not worth writing for a first cut at implementing the new flags. * tools/virsh.c (cmdSnapshotDelete): Add --children-only, --metadata. * tools/virsh.pod (snapshot-delete): Document them. --- tools/virsh.c | 15 ++++++++++++++- tools/virsh.pod | 12 ++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index db6c71e..c01edc1 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12737,6 +12737,9 @@ static const vshCmdOptDef opts_snapshot_delete[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, {"children", VSH_OT_BOOL, 0, N_("delete snapshot and all children")}, + {"children-only", VSH_OT_BOOL, 0, N_("delete children but not snapshot")}, + {"metadata", VSH_OT_BOOL, 0, + N_("delete only libvirt metadata, leaving snapshot contents behind")}, {NULL, 0, 0, NULL} }; @@ -12761,13 +12764,23 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "children")) flags |= VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN; + if (vshCommandOptBool(cmd, "children-only")) + flags |= VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY; + if (vshCommandOptBool(cmd, "metadata")) + flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY; snapshot = virDomainSnapshotLookupByName(dom, name, 0); if (snapshot == NULL) goto cleanup; + /* XXX If we wanted, we could emulate DELETE_CHILDREN_ONLY even on + * older servers that reject the flag, by manually computing the + * list of descendants. But that's a lot of code to maintain. */ if (virDomainSnapshotDelete(snapshot, flags) == 0) { - vshPrint(ctl, _("Domain snapshot %s deleted\n"), name); + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) + vshPrint(ctl, _("Domain snapshot %s children deleted\n"), name); + else + vshPrint(ctl, _("Domain snapshot %s deleted\n"), name); } else { vshError(ctl, _("Failed to delete snapshot %s"), name); goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 0b319ee..1cbe5c3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1645,12 +1645,20 @@ snapshot was taken will be lost. Also note that the state of the domain after snapshot-revert is complete will be the state of the domain at the time the original snapshot was taken. -=item B<snapshot-delete> I<domain> I<snapshot> I<--children> +=item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>] +[{I<--children> | I<--children-only>}] Delete the snapshot for the domain named I<snapshot>. If this snapshot has child snapshots, changes from this snapshot will be merged into the children. If I<--children> is passed, then delete this snapshot and any -children of this snapshot. +children of this snapshot. If I<--children-only> is passed, then delete +any children of this snapshot, but leave this snapshot intact. These +two flags are mutually exclusive. + +If I<--metadata> is specified, then only delete the snapshot metadata +maintained by libvirt, while leaving the snapshot contents intact for +access by external tools; otherwise deleting a snapshot also removes +the data contents from that point in time. =back -- 1.7.4.4

The next patch will make snapshot creation more complex, so it's better to avoid repetition of the complexity. * tools/virsh.c (vshSnapshotCreate): New helper function. (cmdSnapshotCreate, cmdSnapshotCreateAs): Use it. --- tools/virsh.c | 119 ++++++++++++++++++++++++--------------------------------- 1 files changed, 50 insertions(+), 69 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index c01edc1..861300d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12098,6 +12098,54 @@ cmdQuit(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } +/* Helper for snapshot-create and snapshot-create-as */ +static bool +vshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, + unsigned int flags, const char *from) +{ + bool ret = false; + virDomainSnapshotPtr snapshot; + char *doc = NULL; + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; + char *name = NULL; + + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + if (snapshot == NULL) + goto cleanup; + + doc = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (!doc) + goto cleanup; + + xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); + if (!xml) + goto cleanup; + + name = virXPathString("string(/domainsnapshot/name)", ctxt); + if (!name) { + vshError(ctl, "%s", + _("Could not find 'name' element in domain snapshot XML")); + goto cleanup; + } + + if (from) + vshPrint(ctl, _("Domain snapshot %s created from '%s'"), name, from); + else + vshPrint(ctl, _("Domain snapshot %s created"), name); + + ret = true; + +cleanup: + VIR_FREE(name); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + if (snapshot) + virDomainSnapshotFree(snapshot); + VIR_FREE(doc); + return ret; +} + /* * "snapshot-create" command */ @@ -12120,11 +12168,6 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *from = NULL; char *buffer = NULL; - virDomainSnapshotPtr snapshot = NULL; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; - char *doc = NULL; - char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12150,39 +12193,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - snapshot = virDomainSnapshotCreateXML(dom, buffer, 0); - if (snapshot == NULL) - goto cleanup; - - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) - goto cleanup; - - xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); - if (!xml) - goto cleanup; - - name = virXPathString("string(/domainsnapshot/name)", ctxt); - if (!name) { - vshError(ctl, "%s", - _("Could not find 'name' element in domain snapshot XML")); - goto cleanup; - } - - vshPrint(ctl, _("Domain snapshot %s created"), name); - if (from) - vshPrint(ctl, _(" from '%s'"), from); - vshPrint(ctl, "\n"); - - ret = true; + ret = vshSnapshotCreate(ctl, dom, buffer, 0, from); cleanup: - VIR_FREE(name); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - if (snapshot) - virDomainSnapshotFree(snapshot); - VIR_FREE(doc); VIR_FREE(buffer); if (dom) virDomainFree(dom); @@ -12213,13 +12226,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; char *buffer = NULL; - virDomainSnapshotPtr snapshot = NULL; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; - char *doc = NULL; const char *name = NULL; const char *desc = NULL; - char *parsed_name = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -12254,36 +12262,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - snapshot = virDomainSnapshotCreateXML(dom, buffer, 0); - if (snapshot == NULL) - goto cleanup; - - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) - goto cleanup; - - xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); - if (!xml) - goto cleanup; - - parsed_name = virXPathString("string(/domainsnapshot/name)", ctxt); - if (!parsed_name) { - vshError(ctl, "%s", - _("Could not find 'name' element in domain snapshot XML")); - goto cleanup; - } - - vshPrint(ctl, _("Domain snapshot %s created\n"), name ? name : parsed_name); - - ret = true; + ret = vshSnapshotCreate(ctl, dom, buffer, 0, NULL); cleanup: - VIR_FREE(parsed_name); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - if (snapshot) - virDomainSnapshotFree(snapshot); - VIR_FREE(doc); VIR_FREE(buffer); if (dom) virDomainFree(dom); -- 1.7.4.4

No one uses this yet, but it will be important once virDomainSnapshotCreateXML learns a VIR_DOMAIN_SNAPSHOT_DISK_ONLY flag, and the xml allows passing in the new file names. * src/qemu/qemu_monitor.h (qemuMonitorDiskSnapshot): New prototype. * src/qemu/qemu_monitor_text.h (qemuMonitorTextDiskSnapshot): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskSnapshot): Likewise. * src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): New function. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot): Likewise. --- src/qemu/qemu_monitor.c | 24 ++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ src/qemu/qemu_monitor_text.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 4 ++++ 6 files changed, 109 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index db6107c..efc49c4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2391,6 +2391,30 @@ int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name) return ret; } +/* Use the snapshot_blkdev command to convert the existing file for + * device into a read-only backing file of a new qcow2 image located + * at file. */ +int +qemuMonitorDiskSnapshot(qemuMonitorPtr mon, const char *device, + const char *file) +{ + int ret; + + VIR_DEBUG("mon=%p, device=%s, file=%s", mon, device, file); + + if (!mon) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (mon->json) + ret = qemuMonitorJSONDiskSnapshot(mon, device, file); + else + ret = qemuMonitorTextDiskSnapshot(mon, device, file); + return ret; +} + int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f241c9e..b988a72 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -447,6 +447,10 @@ int qemuMonitorCreateSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorLoadSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name); +int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, + const char *device, + const char *file); + int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 715b26e..f8a1696 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2703,6 +2703,39 @@ cleanup: return ret; } +int +qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, const char *device, + const char *file) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("snapshot-blkdev-sync", + "s:device", device, + "s:snapshot-file", file, + NULL); + if (!cmd) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + + if (qemuMonitorJSONHasError(reply, "CommandNotFound") && + qemuMonitorCheckHMP(mon, "snapshot_blkdev")) { + VIR_DEBUG("snapshot-blkdev-sync command not found, trying HMP"); + ret = qemuMonitorTextDiskSnapshot(mon, device, file); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply); + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, char **reply_str, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 9512793..a538e9f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -210,6 +210,10 @@ int qemuMonitorJSONCreateSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorJSONLoadSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorJSONDeleteSnapshot(qemuMonitorPtr mon, const char *name); +int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, + const char *device, + const char *file); + int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, char **reply_str, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 54541ce..3742cdf 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2791,6 +2791,46 @@ 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)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to take snapshot using command '%s'"), cmd); + goto cleanup; + } + + if (strstr(reply, "error while creating qcow2") != NULL) { + qemuReportError(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 b250738..55f78b4 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -206,6 +206,10 @@ 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.4.4

Adds an optional element to <domainsnapshot>, which will be used to give user control over external snapshot filenames on input, and specify generated filenames on output. <domainsnapshot> ... <disks> <disk name='vda' snapshot='no'/> <disk name='vdb' snapshot='internal'/> <disk name='vdc' snapshot='external'> <driver type='qcow2'/> <source file='/path/to/new'/> </disk> </disks> <domain> ... <devices> <disk ...> <driver name='qemu' type='raw'/> <target dev='vdc'/> <source file='/path/to/old'/> </disk> </devices> </domain> </domainsnapshot> * src/conf/domain_conf.h (_virDomainSnapshotDiskDef): New type. (_virDomainSnapshotDef): Add new elements. (virDomainSnapshotAlignDisks): New prototype. * src/conf/domain_conf.c (virDomainSnapshotDiskDefClear) (virDomainSnapshotDiskDefParseXML, disksorter) (virDomainSnapshotAlignDisks): New functions. (virDomainSnapshotDefParseString): Parse new fields. (virDomainSnapshotDefFree): Clean them up. (virDomainSnapshotDefFormat): Output them. * src/libvirt_private.syms (domain_conf.h): Export new function. * docs/schemas/domainsnapshot.rng (domainsnapshot, disksnapshot): Add more xml. * docs/formatsnapshot.html.in: Document it. * tests/domainsnapshotxml2xmlin/disk_snapshot.xml: New test. * tests/domainsnapshotxml2xmlout/disk_snapshot.xml: Update. --- docs/formatsnapshot.html.in | 126 +++++++++- docs/schemas/domainsnapshot.rng | 52 ++++ src/conf/domain_conf.c | 274 ++++++++++++++++++++++ src/conf/domain_conf.h | 22 ++- src/libvirt_private.syms | 1 + tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 16 ++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 42 ++++ 7 files changed, 522 insertions(+), 11 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 4158a63..953272c 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -64,10 +64,10 @@ <p> Attributes of libvirt snapshots are stored as child elements of the <code>domainsnapshot</code> element. At snapshot creation - time, only the <code>name</code> and <code>description</code> - elements are settable; the rest of the fields are informational - (and readonly) and will be filled in by libvirt when the - snapshot is created. + time, only the <code>name</code>, <code>description</code>, + and <code>disks</code> elements are settable; the rest of the + fields are informational (and readonly) and will be filled in by + libvirt when the snapshot is created. </p> <p> The top-level <code>domainsnapshot</code> element may contain @@ -86,6 +86,58 @@ description is omitted when initially creating the snapshot, then this field will be empty. </dd> + <dt><code>disks</code></dt> + <dd>On input, this is an optional listing of specific + instructions for disk snapshots; it is needed when making a + snapshot of only a subset of the disks associated with a + domain, or when overriding the domain defaults for how to + snapshot each disk, or for providing specific control over + what file name is created in an external snapshot. On output, + this is fully populated to show the state of each disk in the + snapshot, including any properties that were generated by the + hypervisor defaults. For system checkpoints, this field is + ignored on input and omitted on output (a system checkpoint + implies that all disks participate in the snapshot process, + and since the current implementation only does internal system + checkpoints, there are no extra details to add); a future + release may allow the use of <code>disks</code> with a system + checkpoint. This element has a list of <code>disk</code> + sub-elements, describing anywhere from zero to all of the + disks associated with the domain. <span class="since">Since + 0.9.5</span> + <dl> + <dt><code>disk</code></dt> + <dd>This sub-element describes the snapshot properties of a + specific disk. The attribute <code>name</code> is + mandatory, and must match the <code>&lt;target + dev='name'/&gt;</code> of one of + the <a href="formatdomain.html#elementsDisks">disk + devices</a> specified for the domain at the time of the + snapshot. The attribute <code>snapshot</code> is + optional, and has the same values of the disk device + element for a domain + (<code>no</code>, <code>internal</code>, + or <code>external</code>). Some hypervisors like ESX + require that if specified, the snapshot mode must not + override any snapshot mode attached to the corresponding + domain disk, while others like qemu allow this field to + override the domain default. If the snapshot mode is + external (whether specified or inherited), then there is + an optional sub-element <code>source</code>, with an + attribute <code>file</code> giving the name, and an + optional sub-element <code>driver</code>, with an + attribute <code>type</code> giving the driver type (such + as qcow2), of the new file created by the external + snapshot of the new file. If <code>source</code> is not + given, a file name is generated that consists of the + existing file name with anything after the trailing dot + replaced by the snapshot name. Remember that with external + snapshots, the original file name becomes the read-only + snapshot, and the new file name contains the read-write + delta of all disk changes since the snapshot. + </dd> + </dl> + </dd> <dt><code>creationTime</code></dt> <dd>The time this snapshot was created. The time is specified in seconds since the Epoch, UTC (i.e. Unix time). Readonly. @@ -124,14 +176,21 @@ <h2><a name="example">Examples</a></h2> - <p>Using this XML on creation:</p> + <p>Using this XML to create a disk snapshot of just vda on a qemu + domain with two disks:</p> <pre> - &lt;domainsnapshot&gt; - &lt;description&gt;Snapshot of OS install and updates&lt;/description&gt; - &lt;/domainsnapshot&gt;</pre> +&lt;domainsnapshot&gt; + &lt;description&gt;Snapshot of OS install and updates&lt;/description&gt; + &lt;disks&gt; + &lt;disk name='vda'&gt; + &lt;source file='/path/to/new'/&gt; + &lt;/disk&gt; + &lt;disk name='vdb' snapshot='no'/&gt; + &lt;/disks&gt; +&lt;/domainsnapshot&gt;</pre> - <p>Will result in XML similar to this from - virDomainSnapshotGetXMLDesc:</p> + <p>will result in XML similar to this from + <code>virDomainSnapshotGetXMLDesc()</code>:</p> <pre> &lt;domainsnapshot&gt; &lt;name&gt;1270477159&lt;/name&gt; @@ -141,14 +200,61 @@ &lt;parent&gt; &lt;name&gt;bare-os-install&lt;/name&gt; &lt;/parent&gt; + &lt;disks&gt; + &lt;disk name='vda' snapshot='external'&gt; + &lt;driver type='qcow2'/&gt; + <b>&lt;source file='/path/to/new'/&gt;</b> + &lt;/disk&gt; + &lt;disk name='vdb' snapshot='no'/&gt; + &lt;/disks&gt; &lt;domain&gt; &lt;name&gt;fedora&lt;/name&gt; &lt;uuid&gt;93a5c045-6457-2c09-e56c-927cdf34e178&lt;/uuid&gt; &lt;memory&gt;1048576&lt;/memory&gt; ... + &lt;devices&gt; + &lt;disk type='file' device='disk'&gt; + &lt;driver name='qemu' type='raw'/&gt; + <b>&lt;source file='/path/to/old'/&gt;</b> + &lt;target dev='vda' bus='virtio'/&gt; + &lt;/disk&gt; + &lt;disk type='file' device='disk' snapshot='external'&gt; + &lt;driver name='qemu' type='raw'/&gt; + &lt;source file='/path/to/old2'/&gt; + &lt;target dev='vdb' bus='virtio'/&gt; + &lt;/disk&gt; + ... &lt;/devices&gt; &lt;/domain&gt; &lt;/domainsnapshot&gt;</pre> + <p>With that snapshot created, <code>/path/to/old</code> is the + read-only backing file to the new active + file <code>/path/to/new</code>. The <code>&lt;domain&gt;</code> + element within the snapshot xml records the state of the domain + just before the snapshot; a call + to <code>virDomainGetXMLDesc()</code> will show that the domain + has been changed to reflect the snapshot: + </p> + <pre> +&lt;domain&gt; + &lt;name&gt;fedora&lt;/name&gt; + &lt;uuid&gt;93a5c045-6457-2c09-e56c-927cdf34e178&lt;/uuid&gt; + &lt;memory&gt;1048576&lt;/memory&gt; + ... + &lt;devices&gt; + &lt;disk type='file' device='disk'&gt; + &lt;driver name='qemu' type='qcow2'/&gt; + <b>&lt;source file='/path/to/new'/&gt;</b> + &lt;target dev='vda' bus='virtio'/&gt; + &lt;/disk&gt; + &lt;disk type='file' device='disk' snapshot='external'&gt; + &lt;driver name='qemu' type='raw'/&gt; + &lt;source file='/path/to/old2'/&gt; + &lt;target dev='vdb' bus='virtio'/&gt; + &lt;/disk&gt; + ... + &lt;/devices&gt; +&lt;/domain&gt;</pre> </body> </html> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 130dad9..671fbe0 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -31,6 +31,13 @@ </element> </optional> <optional> + <element name='disks'> + <zeroOrMore> + <ref name='disksnapshot'/> + </zeroOrMore> + </element> + </optional> + <optional> <element name='active'> <choice> <value>0</value> @@ -72,4 +79,49 @@ </choice> </define> + <define name='disksnapshot'> + <element name='disk'> + <attribute name='name'> + <ref name='deviceName'/> + </attribute> + <choice> + <attribute name='snapshot'> + <value>no</value> + </attribute> + <attribute name='snapshot'> + <value>internal</value> + </attribute> + <group> + <optional> + <attribute name='snapshot'> + <value>external</value> + </attribute> + </optional> + <interleave> + <optional> + <element name='driver'> + <optional> + <attribute name='type'> + <ref name='genericName'/> + </attribute> + </optional> + <empty/> + </element> + </optional> + <optional> + <element name='source'> + <optional> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + </optional> + <empty/> + </element> + </optional> + </interleave> + </group> + </choice> + </element> + </define> + </grammar> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0713a25..d663a01 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10987,18 +10987,82 @@ cleanup: } /* Snapshot Def functions */ +static void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) +{ + VIR_FREE(disk->name); + VIR_FREE(disk->file); + VIR_FREE(disk->driverType); +} + void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) { + int i; + if (!def) return; VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + for (i = 0; i < def->ndisks; i++) + virDomainSnapshotDiskDefClear(&def->disks[i]); + VIR_FREE(def->disks); virDomainDefFree(def->dom); VIR_FREE(def); } +static int +virDomainSnapshotDiskDefParseXML(xmlNodePtr node, + virDomainSnapshotDiskDefPtr def) +{ + int ret = -1; + char *snapshot = NULL; + xmlNodePtr cur; + + def->name = virXMLPropString(node, "name"); + if (!def->name) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name from disk snapshot element")); + goto cleanup; + } + + snapshot = virXMLPropString(node, "snapshot"); + if (snapshot) { + def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + if (def->snapshot <= 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot setting '%s'"), + snapshot); + goto cleanup; + } + } + + cur = node->children; + while (cur) { + if (cur->type == XML_ELEMENT_NODE) { + if (!def->file && + xmlStrEqual(cur->name, BAD_CAST "source")) { + def->file = virXMLPropString(cur, "file"); + } else if (!def->driverType && + xmlStrEqual(cur->name, BAD_CAST "driver")) { + def->driverType = virXMLPropString(cur, "type"); + } + } + cur = cur->next; + } + + if (!def->snapshot && (def->file || def->driverType)) + def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; + + ret = 0; +cleanup: + VIR_FREE(snapshot); + if (ret < 0) + virDomainSnapshotDiskDefClear(def); + return ret; +} + /* If newSnapshot is true, caps, expectedVirtTypes, and flags are ignored. */ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, @@ -11011,6 +11075,8 @@ virDomainSnapshotDefParseString(const char *xmlStr, xmlDocPtr xml = NULL; virDomainSnapshotDefPtr def = NULL; virDomainSnapshotDefPtr ret = NULL; + xmlNodePtr *nodes = NULL; + int i; char *creation = NULL, *state = NULL; struct timeval tv; int active; @@ -11044,6 +11110,19 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup; + def->ndisks = i; + if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + goto cleanup; + } + VIR_FREE(nodes); + if (!newSnapshot) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { @@ -11106,6 +11185,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, cleanup: VIR_FREE(creation); VIR_FREE(state); + VIR_FREE(nodes); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -11114,12 +11194,178 @@ cleanup: return ret; } +static int +disksorter(const void *a, const void *b) +{ + const virDomainSnapshotDiskDef *diska = a; + const virDomainSnapshotDiskDef *diskb = b; + + /* Integer overflow shouldn't be a problem here. */ + return diska->index - diskb->index; +} + +/* Align def->disks to def->domain. Sort the list of def->disks, + * filling in any missing disks or snapshot state defaults given by + * the domain, with a fallback to a passed in default. Issue an error + * and return -1 if any def->disks[n]->name appears more than once or + * does not map to dom->disks. If require_match, also require that + * existing def->disks snapshot states do not override explicit + * def->dom settings. */ +int +virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, + int default_snapshot, + bool require_match) +{ + int ret = -1; + virBitmapPtr map = NULL; + int i; + int ndisks; + bool inuse; + + if (!def->dom) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + + if (def->ndisks > def->dom->ndisks) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("too many disk snapshot requests for domain")); + goto cleanup; + } + + /* Unlikely to have a guest without disks but technically possible. */ + if (!def->dom->ndisks) { + ret = 0; + goto cleanup; + } + + if (!(map = virBitmapAlloc(def->dom->ndisks))) { + virReportOOMError(); + goto cleanup; + } + + /* Double check requested disks. */ + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + int idx = virDomainDiskIndexByName(def->dom, disk->name); + int disk_snapshot; + + if (idx < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no disk named '%s'"), disk->name); + goto cleanup; + } + disk_snapshot = def->dom->disks[idx]->snapshot; + + if (virBitmapGetBit(map, idx, &inuse) < 0 || inuse) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' specified twice"), + disk->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(map, idx)); + disk->index = idx; + if (!disk_snapshot) + disk_snapshot = default_snapshot; + if (!disk->snapshot) { + disk->snapshot = disk_snapshot; + } else if (disk_snapshot && require_match && + disk->snapshot != disk_snapshot) { + const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot); + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' must use snapshot mode '%s'"), + disk->name, tmp); + goto cleanup; + } + if (disk->file && + disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("file '%s' for disk '%s' requires " + "use of external snapshot mode"), + disk->file, disk->name); + goto cleanup; + } + } + + /* Provide defaults for all remaining disks. */ + ndisks = def->ndisks; + if (VIR_EXPAND_N(def->disks, def->ndisks, + def->dom->ndisks - def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < def->dom->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk; + + ignore_value(virBitmapGetBit(map, i, &inuse)); + if (inuse) + continue; + disk = &def->disks[ndisks++]; + if (!(disk->name = strdup(def->dom->disks[i]->dst))) { + virReportOOMError(); + goto cleanup; + } + disk->index = i; + disk->snapshot = def->dom->disks[i]->snapshot; + if (!disk->snapshot) + disk->snapshot = default_snapshot; + } + + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); + + /* Generate any default external file names. */ + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (disk->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL && + !disk->file) { + const char *original = def->dom->disks[i]->src; + const char *tmp; + + if (!original) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external backup name " + "for disk '%s' without source"), + disk->name); + goto cleanup; + } + tmp = strrchr(original, '.'); + if (!tmp || strchr(tmp, '/')) { + ignore_value(virAsprintf(&disk->file, "%s.%s", + original, def->name)); + } else { + if ((tmp - original) > INT_MAX) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("integer overflow")); + goto cleanup; + } + ignore_value(virAsprintf(&disk->file, "%.*s.%s", + (int) (tmp - original), original, + def->name)); + } + if (!disk->file) { + virReportOOMError(); + goto cleanup; + } + } + } + + ret = 0; + +cleanup: + virBitmapFree(map); + return ret; +} + char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal) { virBuffer buf = VIR_BUFFER_INITIALIZER; + int i; virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); @@ -11139,6 +11385,34 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); + /* For now, only output <disks> on disk-snapshot */ + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + virBufferAddLit(&buf, " <disks>\n"); + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (!disk->name) + continue; + + virBufferEscapeString(&buf, " <disk name='%s'", disk->name); + if (disk->snapshot) + virBufferAsprintf(&buf, " snapshot='%s'", + virDomainDiskSnapshotTypeToString(disk->snapshot)); + if (disk->file || disk->driverType) { + virBufferAddLit(&buf, ">\n"); + if (disk->file) + virBufferEscapeString(&buf, " <source file='%s'/>\n", + disk->file); + if (disk->driverType) + virBufferEscapeString(&buf, " <driver type='%s'/>\n", + disk->driverType); + virBufferAddLit(&buf, " </disk>\n"); + } else { + virBufferAddLit(&buf, "/>\n"); + } + } + virBufferAddLit(&buf, " </disks>\n"); + } if (def->dom) { virDomainDefFormatInternal(def->dom, flags, &buf); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ea8194a..d835f96 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1308,7 +1308,20 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_LAST }; -/* Snapshot state */ +/* Items related to snapshot state */ + +/* Stores disk-snapshot information */ +typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; +typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; +struct _virDomainSnapshotDiskDef { + char *name; /* name matching the <target dev='...' of the domain */ + int index; /* index within snapshot->dom->disks that matches name */ + int snapshot; /* enum virDomainDiskSnapshot */ + char *file; /* new source file when snapshot is external */ + char *driverType; /* file format type of new file */ +}; + +/* Stores the complete snapshot metadata */ typedef struct _virDomainSnapshotDef virDomainSnapshotDef; typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; struct _virDomainSnapshotDef { @@ -1318,6 +1331,10 @@ struct _virDomainSnapshotDef { char *parent; long long creationTime; /* in seconds */ int state; /* enum virDomainSnapshotState */ + + size_t ndisks; /* should not exceed dom->ndisks */ + virDomainSnapshotDiskDef *disks; + virDomainDefPtr dom; /* Internal use. */ @@ -1351,6 +1368,9 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal); +int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, + int default_snapshot, + bool require_match); virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 034443c..a953326 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -381,6 +381,7 @@ virDomainSmartcardDefForeach; virDomainSmartcardDefFree; virDomainSmartcardTypeFromString; virDomainSmartcardTypeToString; +virDomainSnapshotAlignDisks; virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml new file mode 100644 index 0000000..1f0beb6 --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -0,0 +1,16 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <disks> + <disk name='hda'/> + <disk name='hdb' snapshot='no'/> + <disk name='hdc' snapshot='internal'/> + <disk name='hdd' snapshot='external'> + <source/> + <driver type='qed'/> + </disk> + <disk name='hde' snapshot='external'> + <source file='/path/to/new'/> + </disk> + </disks> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 391bb57..e0414a1 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -6,6 +6,23 @@ </parent> <state>disk-snapshot</state> <creationTime>1272917631</creationTime> + <disks> + <disk name='hda' snapshot='no'/> + <disk name='hdb' snapshot='no'/> + <disk name='hdc' snapshot='internal'/> + <disk name='hdd' snapshot='external'> + <driver type='qed'/> + <source file='/path/to/generated4'/> + </disk> + <disk name='hde' snapshot='external'> + <driver type='qcow2'/> + <source file='/path/to/new'/> + </disk> + <disk name='hdf' snapshot='external'> + <driver type='qcow2'/> + <source file='/path/to/generated5'/> + </disk> + </disks> <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> @@ -27,6 +44,31 @@ <target dev='hda' bus='ide'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest3'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='2' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest4'/> + <target dev='hdd' bus='ide'/> + <address type='drive' controller='0' bus='3' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest5'/> + <target dev='hde' bus='ide'/> + <address type='drive' controller='0' bus='4' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest6'/> + <target dev='hdf' bus='ide'/> + <address type='drive' controller='0' bus='5' unit='0'/> + </disk> <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> -- 1.7.4.4

This adds a convenience function to virsh that parses out block information from the domain xml, making it much easier to see what strings can be used in all other contexts that demand a specific block name, especially when given the previous patch that allows using either target or unique source name. As an example on a domain with one disk and an empty cdrom drive: Target Source ------------------------------------------- vda /var/lib/libvirt/images/fedora_12.img hdc - * tools/virsh.c (cmdDomblklist): New function. * tools/virsh.pod (domblklist): Document it. --- tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 10 +++++++ 2 files changed, 91 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d4581f7..fa80a52 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1277,6 +1277,86 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) } /* + * "domblklist" command + */ +static const vshCmdInfo info_domblklist[] = { + {"help", N_("list all domain blocks")}, + {"desc", N_("Get the names of block devices for a domain.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_domblklist[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"inactive", VSH_OT_BOOL, 0, + N_("get inactive rather than running configuration")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdDomblklist(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + bool ret = false; + int flags = 0; + char *xml = NULL; + xmlDocPtr xmldoc = NULL; + xmlXPathContextPtr ctxt = NULL; + int ndisks; + xmlNodePtr *disks = NULL; + int i; + + if (vshCommandOptBool(cmd, "inactive")) + flags |= VIR_DOMAIN_XML_INACTIVE; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + xml = virDomainGetXMLDesc(dom, flags); + if (!xml) + goto cleanup; + + xmldoc = virXMLParseStringCtxt(xml, "domain.xml", &ctxt); + if (!xmldoc) + goto cleanup; + + ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks); + if (ndisks < 0) + goto cleanup; + + vshPrint(ctl, "%-10s %s\n", _("Target"), _("Source")); + vshPrint(ctl, "------------------------------------------------\n"); + + for (i = 0; i < ndisks; i++) { + char *target; + char *source; + + ctxt->node = disks[i]; + target = virXPathString("string(./target/@dev)", ctxt); + if (!target) { + vshError(ctl, "unable to query block list"); + goto cleanup; + } + source = virXPathString("string(./source/@file" + "|./source/@dev" + "|./source/@dir" + "|./source/@name)", ctxt); + vshPrint(ctl, "%-10s %s\n", target, source ? source : "-"); + VIR_FREE(target); + VIR_FREE(source); + } + + ret = 0; + +cleanup: + VIR_FREE(disks); + virDomainFree(dom); + return ret; +} + +/* * "suspend" command */ static const vshCmdInfo info_suspend[] = { @@ -13015,6 +13095,7 @@ static const vshCmdDef domManagementCmds[] = { static const vshCmdDef domMonitoringCmds[] = { {"domblkinfo", cmdDomblkinfo, opts_domblkinfo, info_domblkinfo, 0}, + {"domblklist", cmdDomblklist, opts_domblklist, info_domblklist, 0}, {"domblkstat", cmdDomblkstat, opts_domblkstat, info_domblkstat, 0}, {"domcontrol", cmdDomControl, opts_domcontrol, info_domcontrol, 0}, {"domifstat", cmdDomIfstat, opts_domifstat, info_domifstat, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index c37c9e4..b357330 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -447,6 +447,16 @@ Get block device size info for a domain. A I<block-device> corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain>. +=item B<domblklist> I<domain> [I<--inactive>] + +Print a table showing the names of all block devices associated with +I<domain>, as well as the path to the source of each device. If +I<--inactive> is specified, query the block devices that will be used +on the next boot, rather than those currently in use by a running +domain. Other contexts that require a block device name (such as +I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept +either target or unique source names printed by this command. + =item B<blockpull> I<domain> I<path> [I<bandwidth>] Populate a disk from its backing image. Once all data from its backing -- 1.7.4.4

My RFC for snapshot support [1] proposes several rules for when it is safe to delete or revert to an external snapshot, predicated on the existence of new API flags. These will be incrementally added in future patches, but until then, blindly mishandling a disk snapshot risks corrupting internal state, so it is better to outright reject the attempts until the other pieces are in place, thus incrementally relaxing the restrictions added in this patch. [1] https://www.redhat.com/archives/libvir-list/2011-August/msg00361.html * src/qemu/qemu_driver.c (qemuDomainSnapshotCountExternal): New function. (qemuDomainDestroyFlags, qemuDomainUndefineFlags) (qemuDomainSnapshotDelete): Use it to add safety valve. (qemuDomainRevertToSnapshot): Add safety valve. --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aeb8ad9..5652e62 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1777,6 +1777,19 @@ qemuDomainSnapshotDiscardAll(void *payload, curr->err = err; } +/* Count how many snapshots in a set have external disk snapshots. */ +static void +qemuDomainSnapshotCountExternal(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + int *count = data; + + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + (*count)++; +} + static int qemuDomainDestroyFlags(virDomainPtr dom, unsigned int flags) @@ -1787,6 +1800,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; int nsnapshots; + int external = 0; virCheckFlags(VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL, -1); @@ -1814,6 +1828,16 @@ qemuDomainDestroyFlags(virDomainPtr dom, goto cleanup; } + if ((flags & VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL) && + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotCountExternal, + &external) && + external) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + goto cleanup; + } + rem.driver = driver; rem.vm = vm; rem.metadata_only = !(flags & VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL); @@ -4943,6 +4967,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, char *name = NULL; int ret = -1; int nsnapshots; + int external = 0; virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | @@ -4972,6 +4997,16 @@ qemuDomainUndefineFlags(virDomainPtr dom, goto cleanup; } + if ((flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL) && + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotCountExternal, + &external) && + external) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + goto cleanup; + } + rem.driver = driver; rem.vm = vm; rem.metadata_only = !(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL); @@ -9054,6 +9089,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("revert to external disk snapshot not supported " + "yet")); + goto cleanup; + } + if (vm->current_snapshot) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, @@ -9273,6 +9315,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, struct snap_remove rem; struct snap_reparent rep; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + int external = 0; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -9295,6 +9338,22 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } + if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY)) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && + snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + external++; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) + virDomainSnapshotForEachDescendant(&vm->snapshots, snap, + qemuDomainSnapshotCountExternal, + &external); + if (external) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + goto cleanup; + } + } + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 1.7.4.4

I still need a good solution for snapshot-create-as to list multiple disk options. But this patch is sufficient to test the creation of disk snapshots using only default <disks> for snapshot-create-as, and snapshot-create provides full access to the snapshot process. * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Add --disk-only. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document it. --- tools/virsh.c | 12 ++++++++++-- tools/virsh.pod | 20 +++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index fa80a52..8b522fc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12269,6 +12269,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"xmlfile", VSH_OT_DATA, 0, N_("domain snapshot XML")}, {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, + {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {NULL, 0, 0, NULL} }; @@ -12284,6 +12285,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "halt")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; + if (vshCommandOptBool(cmd, "disk-only")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12333,6 +12337,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"description", VSH_OT_DATA, 0, N_("description of snapshot")}, {"print-xml", VSH_OT_BOOL, 0, N_("print XML document rather than create")}, {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, + {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {NULL, 0, 0, NULL} }; @@ -12350,6 +12355,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "halt")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; + if (vshCommandOptBool(cmd, "disk-only")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12377,9 +12385,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptBool(cmd, "print-xml")) { - if (vshCommandOptBool(cmd, "halt")) { + if (flags) { vshError(ctl, "%s", - _("--print-xml and --halt are mutually exclusive")); + _("--print-xml does not work with --halt or --disk-only")); goto cleanup; } vshPrint(ctl, "%s\n", buffer); diff --git a/tools/virsh.pod b/tools/virsh.pod index b357330..a702520 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1605,26 +1605,36 @@ used to represent properties of snapshots. =over 4 -=item B<snapshot-create> I<domain> [I<xmlfile>] [I<--halt>] +=item B<snapshot-create> I<domain> [I<xmlfile>] [I<--halt>] [I<--disk-only>] Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. The only properties settable for a domain snapshot are the -<name> and <description>; the rest of the fields are ignored, and +<name> and <description>, as well as <disks> if I<--disk-only> is given; +the rest of the fields are ignored, and automatically filled in by libvirt. If I<xmlfile> is completely omitted, then libvirt will choose a value for all fields. If I<--halt> is specified, the domain will be left in an inactive state after the snapshot is created. -=item B<snapshot-create-as> I<domain> [{I<--print-xml> | I<--halt>}] -[I<name>] [I<description>] +If I<--disk-only> is specified, the snapshot will only include disk +state rather than the usual system checkpoint with vm state. Disk +snapshots are faster than full system checkpoints, but reverting to a +disk snapshot may require fsck or journal replays, since it is like +the disk state at the point when the power cord is abruptly pulled; +and mixing I<--halt> and I<--disk-only> loses any data that was not +flushed to disk at the time. + +=item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--halt>] +[I<--disk-only>]} [I<name>] [I<description>] Create a snapshot for domain I<domain> with the given <name> and <description>; if either value is omitted, libvirt will choose a value. If I<--print-xml> is specified, then XML appropriate for I<snapshot-create> is output, rather than actually creating a snapshot. Otherwise, if I<--halt> is specified, the domain will be left in an -inactive state after the snapshot is created. +inactive state after the snapshot is created, and if I<--disk-only> +is specified, the snapshot will not include vm state. =item B<snapshot-current> I<domain> [I<--name>] [I<--security-info>] -- 1.7.4.4

Lots of earlier patches led up to this point - the qemu snapshot_blkdev monitor command can now be controlled by libvirt! Well, insofar as SELinux doesn't prevent qemu from open(O_CREAT) on the files. There's still some followup work before things work with SELinux enforcing, but this patch is big enough to post now. There's still room for other improvements, too (for example, taking a disk snapshot of an inactive domain, by using qemu-img for both internal and external snapshots; wiring up delete and revert control, including additional flags from my RFC; supporting active QED disk snapshots; supporting per-storage-volume snapshots such as LVM or btrfs snapshots; etc.). But this patch is the one that proves the new XML works! * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Wire in active disk snapshots. (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): New functions. --- src/qemu/qemu_driver.c | 290 +++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 274 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5652e62..673d751 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8700,6 +8700,241 @@ cleanup: return ret; } +static int +qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def) +{ + int ret = -1; + int i; + bool found = false; + bool active = virDomainObjIsActive(vm); + struct stat st; + + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + switch (disk->snapshot) { + case VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL: + if (active) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("active qemu domains require external disk " + "snapshots; disk %s requested internal"), + disk->name); + goto cleanup; + } + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("internal snapshot for disk %s unsupported " + "for storage type %s"), + disk->name, + NULLSTR(vm->def->disks[i]->driverType)); + goto cleanup; + } + found = true; + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL: + if (!disk->driverType) { + if (!(disk->driverType = strdup("qcow2"))) { + virReportOOMError(); + goto cleanup; + } + } else if (STRNEQ(disk->driverType, "qcow2")) { + /* XXX We should also support QED */ + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot format for disk %s " + "is unsupported: %s"), + disk->name, disk->driverType); + goto cleanup; + } + if (stat(disk->file, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat for disk %s: %s"), + disk->name, disk->file); + goto cleanup; + } + } else if (!S_ISBLK(st.st_mode)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot file for disk %s already " + "exists and is not a block device: %s"), + disk->name, disk->file); + goto cleanup; + } + found = true; + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_NO: + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT: + default: + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected code path")); + goto cleanup; + } + } + + if (!found) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots require at least one disk to be " + "selected for snapshot")); + goto cleanup; + } + + ret = 0; + +cleanup: + return ret; +} + +/* The domain is expected to hold monitor lock. */ +static int +qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, + virDomainSnapshotDiskDefPtr snap, + virDomainDiskDefPtr disk) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *device = NULL; + char *source = NULL; + char *driverType = NULL; + int ret = -1; + + if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected code path")); + return -1; + } + + if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || + !(source = strdup(snap->file)) || + (STRNEQ_NULLABLE(disk->driverType, "qcow2") && + !(driverType = strdup("qcow2")))) { + virReportOOMError(); + goto cleanup; + } + + /* XXX create new file and set selinux labels */ + ret = qemuMonitorDiskSnapshot(priv->mon, device, source); + virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); + if (ret < 0) + goto cleanup; + + /* Update vm in place to match changes. */ + VIR_FREE(disk->src); + disk->src = source; + source = NULL; + if (driverType) { + VIR_FREE(disk->driverType); + disk->driverType = driverType; + driverType = NULL; + } + + /* XXX Do we also need to update vm->newDef if there are pending + * configuration changes awaiting the next boot? */ + +cleanup: + VIR_FREE(device); + VIR_FREE(source); + VIR_FREE(driverType); + return ret; +} + +/* The domain is expected to be locked and active. */ +static int +qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr *vmptr, + virDomainSnapshotObjPtr snap, + unsigned int flags) +{ + virDomainObjPtr vm = *vmptr; + bool resume = false; + int ret = -1; + int i; + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + return -1; + + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + /* In qemu, snapshot_blkdev on a single disk will pause cpus, + * but this confuses libvirt since notifications are not given + * when qemu resumes. And for multiple disks, libvirt must + * pause externally to get all snapshots to be at the same + * point in time. For simplicitly, we always pause ourselves + * rather than relying on qemu doing pause. + */ + if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, + QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + + resume = true; + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + + /* No way to roll back if first disk succeeds but later disks + * fail. Based on earlier qemuDomainSnapshotDiskPrepare, all + * disks in this list are now either SNAPSHOT_NO, or + * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */ + qemuDomainObjEnterMonitorWithDriver(driver, vm); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + continue; + + ret = qemuDomainSnapshotCreateSingleDiskActive(vm, + &snap->def->disks[i], + vm->def->disks[i]); + if (ret < 0) + break; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) { + virDomainEventPtr event; + + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + virDomainAuditStop(vm, "from-snapshot"); + /* We already filtered the _HALT flag for persistent domains + * only, so this end job never drops the last reference. */ + ignore_value(qemuDomainObjEndJob(driver, vm)); + resume = false; + vm = NULL; + if (event) + qemuDomainEventQueue(driver, event); + } + +cleanup: + if (resume && virDomainObjIsActive(vm) && + qemuProcessStartCPUs(driver, vm, conn, + VIR_DOMAIN_RUNNING_UNPAUSED, + QEMU_ASYNC_JOB_NONE) < 0 && + virGetLastError() == NULL) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("resuming after snapshot failed")); + } + + if (vm) { + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + ret = -1; + if (qemuDomainObjEndJob(driver, vm) == 0) { + /* Only possible if a transient vm quit while our locks were down, + * in which case we don't want to save snapshot metadata. */ + *vmptr = NULL; + ret = -1; + } + } + + return ret; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -8713,7 +8948,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_HALT, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_HALT | + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL); qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8742,27 +8978,39 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - /* in a perfect world, we would allow qemu to tell us this. The problem - * is that qemu only does this check device-by-device; so if you had a - * domain that booted from a large qcow2 device, but had a secondary raw - * device attached, you wouldn't find out that you can't snapshot your - * guest until *after* it had spent the time to snapshot the boot device. - * This is probably a bug in qemu, but we'll work around it here for now. - */ - if (!qemuDomainSnapshotIsAllowed(vm)) - goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + false) < 0) + goto cleanup; + if (qemuDomainSnapshotDiskPrepare(vm, def) < 0) + goto cleanup; + def->state = VIR_DOMAIN_DISK_SNAPSHOT; + } else { + /* In a perfect world, we would allow qemu to tell us this. + * The problem is that qemu only does this check + * device-by-device; so if you had a domain that booted from a + * large qcow2 device, but had a secondary raw device + * attached, you wouldn't find out that you can't snapshot + * your guest until *after* it had spent the time to snapshot + * the boot device. This is probably a bug in qemu, but we'll + * work around it here for now. + */ + if (!qemuDomainSnapshotIsAllowed(vm)) + goto cleanup; - if (def->ndisks) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots not supported yet")); - goto cleanup; + if (def->ndisks) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots require use of " + "VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY")); + goto cleanup; + } + def->state = virDomainObjGetState(vm, NULL); } if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; - snap->def->state = virDomainObjGetState(vm, NULL); snap->def->current = true; if (vm->current_snapshot) { snap->def->parent = strdup(vm->current_snapshot->def->name); @@ -8778,7 +9026,17 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* actually do the snapshot */ - if (!virDomainObjIsActive(vm)) { + 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; + } else if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) goto cleanup; } else { -- 1.7.4.4