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 | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38138fc..3ff9bd6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8583,18 +8583,20 @@ 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

While it is nice that snapshots and saved images remember whether the domain was running or paused, sometimes the restoration phase wants to guarantee a particular state (paused to allow hot-plugging, or running without needing to call resume). This introduces new flags to allow the control, and a later patch will implement the flags for qemu. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SAVE_RUNNING) (VIR_DOMAIN_SAVE_PAUSED, VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) (VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED): New flags. * src/libvirt.c (virDomainSaveFlags, virDomainRestoreFlags) (virDomainManagedSave, virDomainSaveImageDefineXML) (virDomainRevertToSnapshot): Document their use, and enforce mutual exclusion. --- include/libvirt/libvirt.h.in | 12 +++++- src/libvirt.c | 89 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 110bd452..7f68b4c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -960,11 +960,14 @@ int virDomainResume (virDomainPtr domain); /** * virDomainSaveRestoreFlags: - * Flags for use in virDomainSaveFlags(), virDomainManagedSave(), and - * virDomainRestoreFlags(). + * Flags for use in virDomainSaveFlags(), virDomainManagedSave(), + * virDomainRestoreFlags(), and virDomainSaveImageDefineXML(). Not all + * flags apply to all these functions. */ typedef enum { VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache pollution */ + VIR_DOMAIN_SAVE_RUNNING = 1 << 1, /* Favor running over paused */ + VIR_DOMAIN_SAVE_PAUSED = 1 << 2, /* Favor paused over running */ } virDomainSaveRestoreFlags; int virDomainSave (virDomainPtr domain, @@ -2577,6 +2580,11 @@ int virDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags); virDomainSnapshotPtr virDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags); +typedef enum { + VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */ + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ +} virDomainSnapshotRevertFlags; + /* Revert the domain to a point-in-time snapshot. The * state of the guest after this call will be the state * of the guest when the snapshot in question was taken diff --git a/src/libvirt.c b/src/libvirt.c index 4284954..6986e9e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2310,7 +2310,7 @@ error: * * This method will suspend a domain and save its memory contents to * a file on disk. After the call, if successful, the domain is not - * listed as running anymore (this may be a problem). + * listed as running anymore (this ends the life of a transient domain). * Use virDomainRestore() to restore a domain after saving. * * See virDomainSaveFlags() for more control. Also, a save file can @@ -2379,7 +2379,7 @@ error: * * This method will suspend a domain and save its memory contents to * a file on disk. After the call, if successful, the domain is not - * listed as running anymore (this may be a problem). + * listed as running anymore (this ends the life of a transient domain). * Use virDomainRestore() to restore a domain after saving. * * If the hypervisor supports it, @dxml can be used to alter @@ -2394,6 +2394,12 @@ error: * fail if it cannot do so for the given system; this can allow less * pressure on file system cache, but also risks slowing saves to NFS. * + * Normally, the saved state file will remember whether the domain was + * running or paused, and restore defaults to the same state. + * Specifying VIR_DOMAIN_SAVE_RUNNING or VIR_DOMAIN_SAVE_PAUSED in + * @flags will override what state gets saved into the file. These + * two flags are mutually exclusive. + * * A save file can be inspected or modified slightly with * virDomainSaveImageGetXMLDesc() and virDomainSaveImageDefineXML(). * @@ -2425,6 +2431,12 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, goto error; } + if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("running and paused flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainSaveFlags) { int ret; char *absolute_to; @@ -2532,6 +2544,12 @@ error: * fail if it cannot do so for the given system; this can allow less * pressure on file system cache, but also risks slowing saves to NFS. * + * Normally, the saved state file will remember whether the domain was + * running or paused, and restore defaults to the same state. + * Specifying VIR_DOMAIN_SAVE_RUNNING or VIR_DOMAIN_SAVE_PAUSED in + * @flags will override the default read from the file. These two + * flags are mutually exclusive. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2557,6 +2575,12 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, goto error; } + if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("running and paused flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainRestoreFlags) { int ret; char *absolute_from; @@ -2661,7 +2685,7 @@ error: * @conn: pointer to the hypervisor connection * @file: path to saved state file * @dxml: XML config for adjusting guest xml used on restore - * @flags: 0 for now + * @flags: bitwise-OR of virDomainSaveRestoreFlags * * This updates the definition of a domain stored in a saved state * file. @file must be a file created previously by virDomainSave() @@ -2673,6 +2697,13 @@ error: * disk device, to match renaming done as part of backing up the disk * device while the domain is stopped. * + * Normally, the saved state file will remember whether the domain was + * running or paused, and restore defaults to the same state. + * Specifying VIR_DOMAIN_SAVE_RUNNING or VIR_DOMAIN_SAVE_PAUSED in + * @flags will override the default saved into the file; omitting both + * leaves the file's default unchanged. These two flags are mutually + * exclusive. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2698,6 +2729,12 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file, goto error; } + if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("running and paused flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainSaveImageDefineXML) { int ret; char *absolute_file; @@ -7018,7 +7055,9 @@ error: * @domain: pointer to a defined domain * * Launch a defined domain. If the call succeeds the domain moves from the - * defined to the running domains pools. + * defined to the running domains pools. The domain will be paused only + * if restoring from managed state created from a paused domain. For more + * control, see virDomainCreateWithFlags(). * * Returns 0 in case of success, -1 in case of error */ @@ -7064,9 +7103,12 @@ error: * Launch a defined domain. If the call succeeds the domain moves from the * defined to the running domains pools. * - * If the VIR_DOMAIN_START_PAUSED flag is set, the guest domain - * will be started, but its CPUs will remain paused. The CPUs - * can later be manually started using virDomainResume. + * If the VIR_DOMAIN_START_PAUSED flag is set, or if the guest domain + * has a managed save image that requested paused state (see + * virDomainManagedSave()) the guest domain will be started, but its + * CPUs will remain paused. The CPUs can later be manually started + * using virDomainResume(). In all other cases, the guest domain will + * be running. * * If the VIR_DOMAIN_START_AUTODESTROY flag is set, the guest * domain will be automatically destroyed when the virConnectPtr @@ -15386,6 +15428,12 @@ error: * fail if it cannot do so for the given system; this can allow less * pressure on file system cache, but also risks slowing saves to NFS. * + * Normally, the managed saved state will remember whether the domain + * was running or paused, and start will resume to the same state. + * Specifying VIR_DOMAIN_SAVE_RUNNING or VIR_DOMAIN_SAVE_PAUSED in + * @flags will override the default saved into the file. These two + * flags are mutually exclusive. + * * Returns 0 in case of success or -1 in case of failure */ int virDomainManagedSave(virDomainPtr dom, unsigned int flags) @@ -15408,6 +15456,12 @@ int virDomainManagedSave(virDomainPtr dom, unsigned int flags) goto error; } + if ((flags & VIR_DOMAIN_SAVE_RUNNING) && (flags & VIR_DOMAIN_SAVE_PAUSED)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("running and paused flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainManagedSave) { int ret; @@ -15839,10 +15893,22 @@ error: /** * virDomainRevertToSnapshot: * @snapshot: a domain snapshot object - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-OR of virDomainSnapshotRevertFlags * * Revert the domain to a given snapshot. * + * Normally, the domain will revert to the same state the domain was + * in while the snapshot was taken (whether inactive, running, or + * paused), except that disk snapshots default to reverting to + * inactive state. Including VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING in + * @flags overrides the snapshot state to guarantee a running domain + * after the revert; or including VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED in + * @flags guarantees a paused domain after the revert. These two + * flags are mutually exclusive. While a persistent domain does not + * need either flag, it is not possible to revert a transient domain + * into an inactive state, so transient domains require the use of one + * of these two flags. + * * Returns 0 if the creation is successful, -1 on error. */ int @@ -15868,6 +15934,13 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto error; } + if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("running and paused flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainRevertToSnapshot) { int ret = conn->driver->domainRevertToSnapshot(snapshot, flags); if (ret < 0) -- 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, 49 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9b5d0b2..914d2be 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2324,7 +2324,7 @@ cleanup: 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; @@ -2340,6 +2340,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, int fd = -1; int directFlag = 0; virFileDirectFdPtr directFd = NULL; + bool bypass_cache = flags & VIR_DOMAIN_SAVE_BYPASS_CACHE; if (qemuProcessAutoDestroyActive(driver, vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, @@ -2375,6 +2376,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 @@ -2552,7 +2558,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); @@ -2590,8 +2598,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: @@ -2629,7 +2636,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); @@ -2660,8 +2669,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: @@ -3812,15 +3820,17 @@ cleanup: } /* Return -1 on most failures after raising error, -2 if edit was specified - * but xmlin does not represent any changes (no error raised), -3 if corrupt - * image was unlinked (no error raised), and opened fd on success. */ + * but xmlin and state (-1 for no change, 0 for paused, 1 for running) do + * not represent any changes (no error raised), -3 if corrupt image was + * unlinked (no error raised), and opened fd on 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, bool unlink_corrupt) + const char *xmlin, int state, bool edit, + bool unlink_corrupt) { int fd; struct qemud_save_header header; @@ -3898,7 +3908,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); @@ -3906,6 +3917,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, @@ -4068,14 +4081,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, false); + &directFd, dxml, state, false, false); if (fd < 0) goto cleanup; @@ -4138,7 +4159,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - NULL, false, false); + NULL, -1, false, false); if (fd < 0) goto cleanup; @@ -4163,13 +4184,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, false); + dxml, state, true, false); if (fd < 0) { /* Check for special case of no change needed. */ @@ -4194,11 +4222,12 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, goto cleanup; } - if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) { + if (lseek(fd, 0, SEEK_SET) != 0) { 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; @@ -4231,7 +4260,8 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL, false, true); + bypass_cache, &directFd, NULL, -1, false, + true); if (fd < 0) { if (fd == -3) ret = 1; -- 1.7.4.4

On 09/01/2011 10:24 PM, Eric Blake wrote: Transition 5 leaks a freed event pointer to the rest of the function (never a good idea). I'm squashing this in before pushing the portion of series that includes this patch. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 10357e4..e45b29e 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -9011,6 +9011,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (rc < 0) goto endjob; virDomainEventFree(event); + event = NULL; if (was_stopped) { /* Transition 2 */ detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 09/01/2011 10:24 PM, Eric Blake wrote: I don't like copy-and-paste FIXME's; it's too easy to later fix one and not the other. Just because it was already the same code twice doesn't mean I should be the third instance. I'm squashing this in before pushing the set including this patch. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index c8836cd..6dd6826 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -8478,14 +8478,18 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) return 1; } -/* The domain is expected to be locked and inactive. */ +/* The domain is expected to be locked and inactive. Return -1 on normal + * failure, 1 if we skipped a disk due to try_all. */ static int -qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, - virDomainSnapshotObjPtr snap) +qemuDomainSnapshotForEachQcow2(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + const char *op, + bool try_all) { - const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; + const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL }; int ret = -1; int i; + bool skipped = false; qemuimgarg[0] = qemuFindQemuImgBinary(); if (qemuimgarg[0] == NULL) { @@ -8493,6 +8497,7 @@ qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, goto cleanup; } + qemuimgarg[2] = op; qemuimgarg[3] = snap->def->name; for (i = 0; i < vm->def->ndisks; i++) { @@ -8503,6 +8508,15 @@ qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { if (!vm->def->disks[i]->driverType || STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + if (try_all) { + /* Continue on even in the face of error, since other + * disks in this VM may have the same snapshot name. + */ + VIR_WARN("skipping snapshot action on %s", + vm->def->disks[i]->info.alias); + skipped = true; + continue; + } qemuReportError(VIR_ERR_OPERATION_INVALID, _("Disk device '%s' does not support" " snapshotting"), @@ -8512,18 +8526,33 @@ qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, qemuimgarg[4] = vm->def->disks[i]->src; - if (virRun(qemuimgarg, NULL) < 0) + if (virRun(qemuimgarg, NULL) < 0) { + if (try_all) { + VIR_WARN("skipping snapshot action on %s", + vm->def->disks[i]->info.alias); + skipped = true; + continue; + } goto cleanup; + } } } - ret = 0; + ret = skipped ? 1 : 0; cleanup: VIR_FREE(qemuimgarg[0]); return ret; } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + return qemuDomainSnapshotForEachQcow2(vm, snap, "-c", false); +} + /* The domain is expected to be locked and active. */ static int qemuDomainSnapshotCreateActive(virConnectPtr conn, @@ -8873,45 +8902,9 @@ 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; + /* Try all disks, but report failure if we skipped any. */ + int ret = qemuDomainSnapshotForEachQcow2(vm, snap, "-a", true); + return ret > 0 ? -1 : ret; } static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, @@ -9135,42 +9128,15 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, virDomainObjPtr vm, virDomainSnapshotObjPtr snap) { - const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; char *snapFile = NULL; int ret = -1; - int i; qemuDomainObjPrivatePtr priv; virDomainSnapshotObjPtr parentsnap = NULL; if (!virDomainObjIsActive(vm)) { - qemuimgarg[0] = qemuFindQemuImgBinary(); - if (qemuimgarg[0] == NULL) - /* qemuFindQemuImgBinary set the error */ + /* Ignore any skipped disks */ + if (qemuDomainSnapshotForEachQcow2(vm, snap, "-d", true) < 0) 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); @@ -9214,7 +9180,6 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, cleanup: VIR_FREE(snapFile); - VIR_FREE(qemuimgarg[0]); return ret; } -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

A future patch will make it impossible to remove a domain if it would leave behind any libvirt-tracked metadata about snapshots, since stale metadata interferes with a new domain by the same name. But requiring snaphot contents to be deleted before removing a domain is harsh; with qemu, qemu-img can still make use of the contents after the libvirt domain is gone. Therefore, we need an option to get rid of libvirt tracking information, but not the actual contents. For hypervisors that do not track any metadata in libvirt, the implementation is trivial; all remaining hypervisors (really, just qemu) will be dealt with separately. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY): New flag. * src/libvirt.c (virDomainSnapshotDelete): Document it. * src/esx/esx_driver.c (esxDomainSnapshotDelete): Trivially supported when there is no libvirt metadata. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotDelete): Likewise. --- include/libvirt/libvirt.h.in | 3 ++- src/esx/esx_driver.c | 10 +++++++++- src/libvirt.c | 10 +++++++--- src/vbox/vbox_tmpl.c | 10 +++++++++- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7f68b4c..637145b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2594,7 +2594,8 @@ int virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Delete a snapshot */ typedef enum { - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = (1 << 0), + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = (1 << 0), /* Also delete children */ + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = (1 << 1), /* Delete just metadata */ } virDomainSnapshotDeleteFlags; int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index c097651..beeafbd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4543,7 +4543,8 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); if (esxVI_EnsureSession(priv->primary) < 0) { return -1; @@ -4561,6 +4562,13 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) goto cleanup; } + /* ESX snapshots do not require any libvirt metadata, making this + * flag trivial once we know we have a valid snapshot. */ + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { + result = 0; + goto cleanup; + } + if (esxVI_RemoveSnapshot_Task(priv->primary, snapshotTree->snapshot, removeChildren, &task) < 0 || esxVI_WaitForTaskCompletion(priv->primary, task, snapshot->domain->uuid, diff --git a/src/libvirt.c b/src/libvirt.c index 6986e9e..fdf8368 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15957,14 +15957,18 @@ error: /** * virDomainSnapshotDelete: * @snapshot: a domain snapshot object - * @flags: flag parameters + * @flags: bitwise-or of supported virDomainSnapshotDeleteFlags * * Delete the snapshot. * * If @flags is 0, then just this snapshot is deleted, and changes from * this snapshot are automatically merged into children snapshots. If - * flags is VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, then this snapshot - * and any children snapshots are deleted. + * @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, then this snapshot + * and any children snapshots are deleted. If @flags includes + * VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, then any snapshot metadata + * tracked by libvirt is removed while keeping the snapshot contents + * intact; if a hypervisor does not require any libvirt metadata to + * track snapshots, then this flag is silently ignored. * * Returns 0 if the snapshot was successfully deleted, -1 on error. */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 822e899..8de2bae 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6361,7 +6361,8 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, PRUint32 state; nsresult rc; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); @@ -6382,6 +6383,13 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } + /* VBOX snapshots do not require any libvirt metadata, making this + * flag trivial once we know we have a valid snapshot. */ + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { + ret = 0; + goto cleanup; + } + if (state >= MachineState_FirstOnline && state <= MachineState_LastOnline) { vboxError(VIR_ERR_OPERATION_INVALID, "%s", -- 1.7.4.4

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. --- 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 00212db..993d590 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11215,23 +11215,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

Deleting a snapshot and all its descendants had problems with tracking the current snapshot. The deletion does not necessarily proceed in depth-first order, so a parent could be deleted before a child, wreaking havoc on passing the notion of the current snapshot to the parent. Furthermore, even if traversal were depth-first, doing multiple file writes to pass current up the chain one snapshot at a time is wasteful, comparing to a single update to the current snapshot at the end of the algorithm. * src/qemu/qemu_driver.c (snap_remove): Add field. (qemuDomainSnapshotDiscard): Add parameter. (qemuDomainSnapshotDiscardDescendant): Adjust accordingly. (qemuDomainSnapshotDelete): Properly reset current. --- src/qemu/qemu_driver.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 242708c..a96ec84 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9166,9 +9166,11 @@ cleanup: return ret; } -static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap) +static int +qemuDomainSnapshotDiscard(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool update_current) { const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; char *snapFile = NULL; @@ -9221,7 +9223,7 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, } if (snap == vm->current_snapshot) { - if (snap->def->parent) { + if (update_current && snap->def->parent) { parentsnap = virDomainSnapshotFindByName(&vm->snapshots, snap->def->parent); if (!parentsnap) { @@ -9258,6 +9260,7 @@ struct snap_remove { struct qemud_driver *driver; virDomainObjPtr vm; int err; + bool current; }; static void @@ -9269,7 +9272,9 @@ qemuDomainSnapshotDiscardDescendant(void *payload, struct snap_remove *curr = data; int err; - err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap); + if (snap->def->current) + curr->current = true; + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false); if (err && !curr->err) curr->err = err; } @@ -9348,12 +9353,15 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rem.driver = driver; rem.vm = vm; rem.err = 0; + rem.current = false; virDomainSnapshotForEachDescendant(&vm->snapshots, snap, qemuDomainSnapshotDiscardDescendant, &rem); if (rem.err < 0) goto endjob; + if (rem.current) + vm->current_snapshot = snap; } else { rep.driver = driver; rep.snap = snap; @@ -9365,7 +9373,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto endjob; } - ret = qemuDomainSnapshotDiscard(driver, vm, snap); + ret = qemuDomainSnapshotDiscard(driver, vm, snap, true); endjob: if (qemuDomainObjEndJob(driver, vm) == 0) -- 1.7.4.4

Adding this was trivial compared to the previous patch for fixing qemu snapshot deletion in the first place. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiscard): Add parameter. (qemuDomainSnapshotDiscardDescendant, qemuDomainSnapshotDelete): Update callers. --- src/qemu/qemu_driver.c | 78 ++++++++++++++++++++++++++--------------------- 1 files changed, 43 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 85e0f7d..45dd582 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9170,7 +9170,8 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, virDomainObjPtr vm, virDomainSnapshotObjPtr snap, - bool update_current) + bool update_current, + bool metadata_only) { const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; char *snapFile = NULL; @@ -9179,41 +9180,43 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv; virDomainSnapshotObjPtr parentsnap = NULL; - 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 (!metadata_only) { + if (!virDomainObjIsActive(vm)) { + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) + /* qemuFindQemuImgBinary set the error */ + goto cleanup; - 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; + 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); } - } 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, @@ -9259,6 +9262,7 @@ cleanup: struct snap_remove { struct qemud_driver *driver; virDomainObjPtr vm; + bool metadata_only; int err; bool current; }; @@ -9274,7 +9278,8 @@ qemuDomainSnapshotDiscardDescendant(void *payload, if (snap->def->current) curr->current = true; - err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false); + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false, + curr->metadata_only); if (err && !curr->err) curr->err = err; } @@ -9324,8 +9329,10 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, char uuidstr[VIR_UUID_STRING_BUFLEN]; struct snap_remove rem; struct snap_reparent rep; + bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); - virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); @@ -9350,6 +9357,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { rem.driver = driver; rem.vm = vm; + rem.metadata_only = metadata_only; rem.err = 0; rem.current = false; virDomainSnapshotForEachDescendant(&vm->snapshots, @@ -9372,7 +9380,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto endjob; } - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true); + ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); endjob: if (qemuDomainObjEndJob(driver, vm) == 0) -- 1.7.4.4

New flag bits are worth exposing via virsh. 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 --roots, --metadata. * tools/virsh.pod (snapshot-dumpxml, snapshot-current) (snapshot-list): Document these. --- tools/virsh.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 20 +++++++++++++++----- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b0f4319..90f6765 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12110,6 +12110,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} }; @@ -12121,6 +12123,10 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) int current; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12138,7 +12144,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; @@ -12185,6 +12191,9 @@ 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} }; @@ -12194,8 +12203,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; unsigned int flags = 0; - int parent_filter = 0; /* 0 for no parent information needed, - 1 for parent column */ + 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; @@ -12212,7 +12221,18 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) 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)) @@ -12224,6 +12244,16 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) 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; @@ -12271,6 +12301,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent_filter) { parent = virXPathString("string(/domainsnapshot/parent/name)", ctxt); + if (!parent && parent_filter < 0) + continue; } state = virXPathString("string(/domainsnapshot/state)", ctxt); @@ -12328,6 +12360,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} }; @@ -12339,6 +12373,10 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12354,7 +12392,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 7d8e435..cfdfed9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -596,7 +596,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. @@ -1692,21 +1692,31 @@ 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. +If I<--name> is specified, just print the current snapshot name instead +of the full xml. Otherwise, using I<--security-info> will also include +security sensitive information in the XML. -=item B<snapshot-list> I<domain> [I<--parent>] +=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots>}] [I<--metadata>] List all of the available snapshots for the given domain. If I<--parent> is specified, add a column to the output table giving the name of the parent of each snapshot. -=item B<snapshot-dumpxml> I<domain> I<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

Filtering for roots is pretty easy to do. * src/conf/domain_conf.h (virDomainSnapshotObjListGetNames) (virDomainSnapshotObjListNum): Update prototype. * src/conf/domain_conf.c (virDomainSnapshotObjListCopyNames) (virDomainSnapshotObjListGetNames, virDomainSnapshotObjListCount) (virDomainSnapshotObjListNum): Support filtering. * src/qemu/qemu_driver.c (qemuDomainSnapshotNum) (qemuDomainSnapshotListNames): Update callers. --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++---------- src/conf/domain_conf.h | 6 ++++-- src/qemu/qemu_driver.c | 11 +++++++---- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3f3d2bb..1b2fd61 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11157,6 +11157,7 @@ struct virDomainSnapshotNameData { int numnames; int maxnames; char **const names; + unsigned int flags; }; static void virDomainSnapshotObjListCopyNames(void *payload, @@ -11168,6 +11169,8 @@ static void virDomainSnapshotObjListCopyNames(void *payload, if (data->oom) return; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS) && obj->def->parent) + return; if (data->numnames < data->maxnames) { if (!(data->names[data->numnames] = strdup(obj->def->name))) @@ -11178,9 +11181,10 @@ static void virDomainSnapshotObjListCopyNames(void *payload, } int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - char **const names, int maxnames) + char **const names, int maxnames, + unsigned int flags) { - struct virDomainSnapshotNameData data = { 0, 0, maxnames, names }; + struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, flags }; int i; virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, &data); @@ -11197,22 +11201,31 @@ cleanup: return -1; } -static void virDomainSnapshotObjListCount(void *payload ATTRIBUTE_UNUSED, +struct virDomainSnapshotNumData { + int count; + unsigned int flags; +}; + +static void virDomainSnapshotObjListCount(void *payload, const void *name ATTRIBUTE_UNUSED, - void *data) + void *opaque) { - int *count = data; + virDomainSnapshotObjPtr obj = payload; + struct virDomainSnapshotNumData *data = opaque; - (*count)++; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS) && obj->def->parent) + return; + data->count++; } -int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots) +int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + unsigned int flags) { - int count = 0; + struct virDomainSnapshotNumData data = { 0, flags }; - virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &count); + virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data); - return count; + return data.count; } virDomainSnapshotObjPtr diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5f752ec..503fb58 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1335,8 +1335,10 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - char **const names, int maxnames); -int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots); + char **const names, int maxnames, + unsigned int flags); +int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + unsigned int flags); virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name); void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dcb3661..840c444 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8676,7 +8676,8 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8688,7 +8689,8 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, goto cleanup; } - n = virDomainSnapshotObjListGetNames(&vm->snapshots, names, nameslen); + n = virDomainSnapshotObjListGetNames(&vm->snapshots, names, nameslen, + flags); cleanup: if (vm) @@ -8704,7 +8706,8 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8720,7 +8723,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our * answer. */ - n = virDomainSnapshotObjListNum(&vm->snapshots); + n = virDomainSnapshotObjListNum(&vm->snapshots, flags); cleanup: if (vm) -- 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 90f6765..f048dd5 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11905,6 +11905,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 */ @@ -11927,11 +11975,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; @@ -11957,39 +12000,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); @@ -12020,13 +12033,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)) @@ -12061,36 +12069,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

Supporting NO_METADATA on snapshot creation is interesting - we must still return a valid opaque snapshot object, but the user can't get anything out of it (unless we add a virDomainSnapshotGetName()), since it is no longer registered with the domain. Also, virsh now tries to query for secure xml, in anticipation of when we store <domain> xml inside <domainsnapshot>; for now, we can trivially support it, since we have nothing secure. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new flag. (qemuDomainSnapshotGetXMLDesc): Trivially support VIR_DOMAIN_XML_SECURE. --- src/qemu/qemu_driver.c | 20 ++++++++++++-------- 1 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 840c444..6869b02 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8584,7 +8584,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8626,11 +8626,13 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, virReportOOMError(); goto cleanup; } - vm->current_snapshot->def->current = false; - if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, - driver->snapshotDir) < 0) - goto cleanup; - vm->current_snapshot = NULL; + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + vm->current_snapshot->def->current = false; + if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, + driver->snapshotDir) < 0) + goto cleanup; + vm->current_snapshot = NULL; + } } /* actually do the snapshot */ @@ -8651,7 +8653,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, cleanup: if (vm) { - if (snapshot) { + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0) VIR_WARN("unable to save metadata for snapshot %s", @@ -8840,7 +8842,9 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virCheckFlags(0, NULL); + /* XXX Actually wire this up once we return domain xml; for now, + * it is trivially safe to ignore this flag. */ + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); -- 1.7.4.4

On 09/01/2011 10:25 PM, Eric Blake wrote: This validation is pretty weak, and can be easily exploited to cause libvirtd to infloop. I'm strengthening it by squashing in the following on this patch (plus later patches, when snapshot->def->dom is added, will add ABI compatibility checking). [Technically, a user can still cause libvirtd infloops by messing directly with files in /var/lib/libvirt/qemu/snapshot/dom/*.xml, but those files are supposed to be protected, and touching them outside of libvirt API is already in unsupported territory - our goal only has to be that the API can't be abused to get into bad state, not to protect ourselves from someone clobbering our internal directories.] diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index f862b81..de13584 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -8664,12 +8664,57 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { - virDomainSnapshotObjPtr prior = NULL; + virDomainSnapshotObjPtr other = NULL; - prior = virDomainSnapshotFindByName(&vm->snapshots, def->name); - if (prior) { + /* Prevent circular chains */ + if (def->parent) { + if (STREQ(def->name, def->parent)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("cannot set snapshot %s as its own parent"), + def->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(&vm->snapshots, def->parent); + if (!other) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("parent %s for snapshot %s not found"), + def->parent, def->name); + goto cleanup; + } + while (other->def->parent) { + if (STREQ(other->def->parent, def->name)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("parent %s would create cycle to %s"), + other->def->name, def->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(&vm->snapshots, + other->def->parent); + if (!other) { + VIR_WARN("snapshots are inconsistent for %s", + vm->def->name); + break; + } + } + } + + /* Check that any replacement is compatible */ + other = virDomainSnapshotFindByName(&vm->snapshots, def->name); + if (other) { + if (other == vm->current_snapshot) + def->current == true; + if ((other->def->state == VIR_DOMAIN_RUNNING || + other->def->state == VIR_DOMAIN_PAUSED) != + (def->state == VIR_DOMAIN_RUNNING || + def->state == VIR_DOMAIN_PAUSED)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("cannot change between online and offline " + "snapshot state in snapshot %s"), + def->name); + goto cleanup; + } /* XXX Ensure ABI compatibility before replacing anything. */ - virDomainSnapshotObjListRemove(&vm->snapshots, prior); + virDomainSnapshotObjListRemove(&vm->snapshots, other); } } -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Hello Eric, On Friday 02 September 2011 06:25:01 Eric Blake wrote: I just noticed a problem with that patch 282fe1f08c89189e36142fc2d12bae0175038bdd: This check restricts the test to only running domains, that is if you undefine a currently inactive domain, its snapshot metadata is left behind while the domain is deleted. This behaviour is actually documented in <http://libvirt.org/html/libvirt-libvirt.html#virDomainUndefineFlags> (now that I know what I have to look at), but I was still surprised that virDomainUndefineFlags(0) returned success on my inactive domain with snapshots. Is this restriction to only delete the snapshots for active domains on purpose or am I missing something? I would expect the undefine to be refused for all states, not just for active domains. I would prefer the snapshots to be deleted for both active and inactive domains, since qemuDomainSnapshotDiscardAllMetadata() is not available externally. And iterating over all snapshots to just delete them seems to be wasteful, especially when you use qcow2 with its reference counting issues. See the attached patch for my proposal. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn(a)univention.de Univention GmbH be open. fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/

On 10/26/2012 09:47 AM, Philipp Hahn wrote: I think you read this backwards. This check says that if the domain is offline, it cannot be undefined, as that would strand the metadata. If the domain is active, then it is running, and all undefine does on a running domain is convert it to transient, but the domain is still running, so the metadata is still accessible (up until the transient domain halts, at which point it is cleaned up then). That shouldn't happen - virDomainUndefineFlags(0) on an inactive domain should fail if the domain has snapshots. Is this something you actually hit, and can you give me steps to reproduce? The undefine is refused only if it would strand data. In the case of an active domain, the snapshots are not stranded because the now-transient domain still exists. I'm still not convinced we need your patch - using undefine on an active domain to convert it to transient is too soon to forcefully discard snapshots, as I could then redefine the domain to make it persistent again at which point the snapshots still make sense to keep around. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Similar to 'undefine --managed-save' (commit 83e849c1), we must assume that the old API is unsafe; however, we cannot emulate metadata-only deletion on older servers. 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. Do the best we can given the server we are talking to. * tools/virsh.c (cmdUndefine): Add --snapshots-metadata flag. * tools/virsh.pod (undefine, destroy, shutdown): Document effect of snapshots. --- tools/virsh.c | 156 ++++++++++++++++++++++++++++++++++++++++-------------- tools/virsh.pod | 25 ++++++++- 2 files changed, 138 insertions(+), 43 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index e6a053b..2fa25ea 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1434,6 +1434,8 @@ 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")}, {NULL, 0, 0, NULL} }; @@ -1441,18 +1443,31 @@ static bool cmdUndefine(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; const char *name = NULL; + /* Flags to attempt. */ unsigned 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"); + /* 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 (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1460,61 +1475,120 @@ 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; + has_managed_save = 0; } - } - - if (flags == -1) { - if (has_managed_save == 1) { - vshError(ctl, - _("Refusing to undefine while domain managed save " - "image exists")); - virDomainFree(dom); - return false; - } - - 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) { + 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; + } - rc = virDomainUndefine(dom); + /* 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; } } -end: + /* No way to emulate deletion of just snapshot metadata + * without support for the newer flags. Oh well. */ + if (has_snapshots_metadata) { + vshError(ctl, + snapshots_metadata ? + _("Unable to remove metadata of %d snapshots") : + _("Refusing to undefine while %d snapshots exist"), + has_snapshots_metadata); + goto cleanup; + } + + 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; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 2c8d66c..e5d6b71 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -496,6 +496,11 @@ 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. +If I<domain-id> is transient, then the metadata of any snapshots will +be lost once the guest stops running, but the snapshot contents still +exist, and a new domain with the same name and UUID can restore the +snapshot metadata with B<snapshot-create>. + =item B<domblkstat> I<domain> I<block-device> Get device block stats for a running domain. @@ -998,6 +1003,11 @@ 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. +If I<domain-id> is transient, then the metadata of any snapshots will +be lost once the guest stops running, but the snapshot contents still +exist, and a new domain with the same name and UUID can restore the +snapshot metadata with B<snapshot-create>. + =item B<start> I<domain-name> [I<--console>] [I<--paused>] [I<--autodestroy>] [I<--bypass-cache>] [I<--force-boot>] @@ -1029,16 +1039,22 @@ 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-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-metadata> flag guarantees that any snapshots (see the +B<snapshot-list> command) are also cleaned up when undefining an inactive +domain. Without the flag, attempts to undefine an inactive domain with +snapshot metadata will fail. If the domain is active, this flag is +ignored. + NOTE: For an inactive domain, the domain name or UUID must be used as the I<domain-id>. @@ -1701,6 +1717,11 @@ treat the snapshot as current, and cannot revert to the snapshot unless I<--redefine> is later used to teach libvirt about the metadata again). +Existence of snapshot metadata will prevent attempts to B<undefine> +a persistent domain. However, for transient domains, snapshot +metadata is silently lost when the domain quits running (whether +by command such as B<destroy> or by internal guest action). + =item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>]} [I<name>] [I<description>] -- 1.7.4.4

As more clients start to want to know this information, doing a PATH stat walk and malloc for every client adds up. * src/qemu/qemu_conf.h (qemud_driver): Add member. * src/qemu/qemu_driver.c (qemudShutdown): Cleanup. (qemuFindQemuImgBinary): Add an argument, and cache result. (qemuDomainSnapshotDiscard, qemuDomainSnapshotCreateInactive) (qemuDomainSnapshotRevertInactive, qemuDomainSnapshotCreateXML) (qemuDomainRevertToSnapshot): Update callers. --- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 42 +++++++++++++++++++++--------------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 0a60d32..5469a63 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -82,6 +82,7 @@ struct qemud_driver { char *cacheDir; char *saveDir; char *snapshotDir; + char *qemuImgBinary; unsigned int vncAutoUnixSocket : 1; unsigned int vncTLS : 1; unsigned int vncTLSx509verify : 1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2db0d62..2c3a5c1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -777,6 +777,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->cacheDir); VIR_FREE(qemu_driver->saveDir); VIR_FREE(qemu_driver->snapshotDir); + VIR_FREE(qemu_driver->qemuImgBinary); VIR_FREE(qemu_driver->autoDumpPath); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); @@ -1583,19 +1584,19 @@ cleanup: /* Locate an appropriate 'qemu-img' binary. */ -static char * -qemuFindQemuImgBinary(void) +static const char * +qemuFindQemuImgBinary(struct qemud_driver *driver) { - 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")); + if (!driver->qemuImgBinary) { + driver->qemuImgBinary = virFindFileInPath("kvm-img"); + if (!driver->qemuImgBinary) + driver->qemuImgBinary = virFindFileInPath("qemu-img"); + if (!driver->qemuImgBinary) + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + } - return ret; + return driver->qemuImgBinary; } static int @@ -1670,7 +1671,7 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver, if (!metadata_only) { if (!virDomainObjIsActive(vm)) { - qemuimgarg[0] = qemuFindQemuImgBinary(); + qemuimgarg[0] = qemuFindQemuImgBinary(driver); if (qemuimgarg[0] == NULL) /* qemuFindQemuImgBinary set the error */ goto cleanup; @@ -1742,7 +1743,6 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver, cleanup: VIR_FREE(snapFile); - VIR_FREE(qemuimgarg[0]); return ret; } @@ -8606,14 +8606,15 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) /* The domain is expected to be locked and inactive. */ static int -qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, +qemuDomainSnapshotCreateInactive(struct qemud_driver *driver, + virDomainObjPtr vm, virDomainSnapshotObjPtr snap) { const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; int ret = -1; int i; - qemuimgarg[0] = qemuFindQemuImgBinary(); + qemuimgarg[0] = qemuFindQemuImgBinary(driver); if (qemuimgarg[0] == NULL) { /* qemuFindQemuImgBinary set the error */ goto cleanup; @@ -8646,7 +8647,6 @@ qemuDomainSnapshotCreateInactive(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(qemuimgarg[0]); return ret; } @@ -8801,7 +8801,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, * qemu-img recognizes the snapshot name in at least one of * the domain's disks? */ } else if (!virDomainObjIsActive(vm)) { - if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) + if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) goto cleanup; } else { if (qemuDomainSnapshotCreateActive(domain->conn, driver, @@ -9038,14 +9038,15 @@ cleanup: /* The domain is expected to be locked and inactive. */ static int -qemuDomainSnapshotRevertInactive(virDomainObjPtr vm, +qemuDomainSnapshotRevertInactive(struct qemud_driver *driver, + virDomainObjPtr vm, virDomainSnapshotObjPtr snap) { const char *qemuimgarg[] = { NULL, "snapshot", "-a", NULL, NULL, NULL }; int ret = -1; int i; - qemuimgarg[0] = qemuFindQemuImgBinary(); + qemuimgarg[0] = qemuFindQemuImgBinary(driver); if (qemuimgarg[0] == NULL) { /* qemuFindQemuImgBinary set the error */ goto cleanup; @@ -9078,7 +9079,6 @@ qemuDomainSnapshotRevertInactive(virDomainObjPtr vm, ret = 0; cleanup: - VIR_FREE(qemuimgarg[0]); return ret; } @@ -9270,7 +9270,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, detail); } - if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) { + if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) { if (!vm->persistent) { if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); -- 1.7.4.4

Migration is another case of stranding metadata. And since snapshot metadata is arbitrarily large, there's no way to shoehorn it into the migration cookie of migration v3. This patch consolidates two existing locations for migration validation into one helper function, then enhances that function to also do the new checks. If we could always trust the source to validate migration, then the destination would not have to do anything; but since older servers that did not do checking can migrate to newer destinations, we have to repeat some of the same checks on the destination; meanwhile, we want to detect failures as soon as possible. With migration v2, this means that validation will reject things at Prepare on the destination if the XML exposes the problem, otherwise at Perform on the source; with migration v3, this means that validation will reject things at Begin on the source, or if the source is old and the XML exposes the problem, then at Prepare on the destination. A future patch will make it possible to manually recreate the snapshot metadata on the destination. But even that is limited, since if we delete the snapshot metadata prior to migration, then we won't know the name of the current snapshot to pass along; and if we delete the snapshot metadata after migration and use the v3 migration cookie to pass along the name of the current snapshot, then we need a way to bypass the fact that this patch refuses migration with snapshot metadata present. So eventually, we may have to introduce migration protocol v4 that allows feature negotiation and an arbitrary number of handshake exchanges, so as to pass as many rpc calls as needed to transfer all the snapshot xml hierarchy. But all of that is thoughts for the future; for now, the best course of action is to quit early, rather than get into a funky state of stale metadata. * src/qemu/qemu_migration.h (qemuMigrationIsAllowed): Make static. * src/qemu/qemu_migration.c (qemuMigrationIsAllowed): Alter signature, and allow checks for both outgoing and incoming. (qemuMigrationBegin, qemuMigrationPrepareAny) (qemuMigrationPerformJob): Update callers. --- src/qemu/qemu_migration.c | 48 ++++++++++++++++++++++++++++++-------------- src/qemu/qemu_migration.h | 2 - 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d239cc8..f849d05 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -701,9 +701,36 @@ error: return NULL; } -bool -qemuMigrationIsAllowed(virDomainDefPtr def) +/* Validate whether the domain is safe to migrate. If vm is NULL, + * then this is being run in the v2 Prepare stage on the destination + * (where we only have the target xml); if vm is provided, then this + * is being run in either v2 Perform or v3 Begin (where we also have + * access to all of the domain's metadata, such as whether it is + * marked autodestroy or has snapshots). While it would be nice to + * assume that checking on source is sufficient to prevent ever + * talking to the destination in the first place, we are stuck with + * the fact that older servers did not do checks on the source. */ +static bool +qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, + virDomainDefPtr def) { + int nsnapshots; + + if (vm) { + if (qemuProcessAutoDestroyActive(driver, vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is marked for auto destroy")); + return false; + } + if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain with %d snapshots"), + nsnapshots); + return false; + } + + def = vm->def; + } if (def->nhostdevs > 0) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain with assigned host devices cannot be migrated")); @@ -915,13 +942,7 @@ char *qemuMigrationBegin(struct qemud_driver *driver, if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_BEGIN3); - if (qemuProcessAutoDestroyActive(driver, vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is marked for auto destroy")); - goto cleanup; - } - - if (!qemuMigrationIsAllowed(vm->def)) + if (!qemuMigrationIsAllowed(driver, vm, NULL)) goto cleanup; if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0))) @@ -990,7 +1011,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - if (!qemuMigrationIsAllowed(def)) + if (!qemuMigrationIsAllowed(driver, NULL, def)) goto cleanup; /* Target domain name, maybe renamed. */ @@ -2194,11 +2215,8 @@ qemuMigrationPerformJob(struct qemud_driver *driver, goto endjob; } - if (qemuProcessAutoDestroyActive(driver, vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is marked for auto destroy")); - goto endjob; - } + if (!qemuMigrationIsAllowed(driver, vm, NULL)) + goto cleanup; resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index ace411d..ec70422 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -73,8 +73,6 @@ bool qemuMigrationJobIsActive(virDomainObjPtr vm, int qemuMigrationJobFinish(struct qemud_driver *driver, virDomainObjPtr obj) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -bool qemuMigrationIsAllowed(virDomainDefPtr def) - ATTRIBUTE_NONNULL(1); int qemuMigrationSetOffline(struct qemud_driver *driver, virDomainObjPtr vm); -- 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 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 | 30 ++++++++++++++++++++----- src/conf/domain_conf.c | 50 ++++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 4 +++ src/esx/esx_driver.c | 4 +- src/libvirt.c | 7 +++++- src/qemu/qemu_driver.c | 21 +++++++++++++----- src/vbox/vbox_tmpl.c | 4 +- 7 files changed, 98 insertions(+), 22 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index e43d192..1e2d28a 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -63,18 +63,32 @@ snapshots, as described above. 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;/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; @@ -82,7 +96,11 @@ &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> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bdea5a9..7645628 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10955,12 +10955,19 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + virDomainDefFree(def->dom); VIR_FREE(def); } -/* flags are from virDomainSnapshotParseFlags */ +/* flags is bitwise-or of virDomainSnapshotParseFlags. + * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then + * caps and expectedVirtTypes are ignored. + */ virDomainSnapshotDefPtr -virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags) +virDomainSnapshotDefParseString(const char *xmlStr, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -10969,6 +10976,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags) char *creation = NULL, *state = NULL; struct timeval tv; int active; + char *tmp; xml = virXMLParseCtxt(NULL, xmlStr, "domainsnapshot.xml", &ctxt); if (!xml) { @@ -11032,6 +11040,29 @@ virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags) state); goto cleanup; } + + /* 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))) { + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + + VIR_FREE(tmp); + if (!domainNode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + def->dom = virDomainDefParseNode(caps, xml, domainNode, + expectedVirtTypes, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); + if (!def->dom) + goto cleanup; + } else { + VIR_WARN("parsing older snapshot that lacks domain"); + } } else { def->creationTime = tv.tv_sec; } @@ -11060,10 +11091,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) @@ -11078,9 +11114,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 5e4c67e..54244be 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 */ @@ -1330,10 +1331,13 @@ typedef enum { } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, + 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 fb5b1a2..1458828 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4220,7 +4220,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 0); + def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0); if (def == NULL) { return NULL; @@ -4316,7 +4316,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 68d469f..3ab2ea5 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15693,10 +15693,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 e5eb8b6..8fafade 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -340,7 +340,9 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, flags); + def = virDomainSnapshotDefParseString(xmlStr, qemu_driver->caps, + QEMU_EXPECTED_VIRT_TYPES, + flags); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -1612,7 +1614,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; @@ -1638,6 +1641,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); @@ -8769,7 +8778,9 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, parse_flags))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, + QEMU_EXPECTED_VIRT_TYPES, + parse_flags))) goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { @@ -9020,8 +9031,6 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - /* XXX Actually wire this up once we return domain xml; for now, - * it is trivially safe to ignore this flag. */ virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); qemuDriverLock(driver); @@ -9041,7 +9050,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 ebed4d9..cbe34e8 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5658,7 +5658,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 0))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); @@ -5840,7 +5840,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

Commit 69278878 fixed one direction of arbitrarily-named snapshots, but not the round trip path. While auditing domain_conf, I found a couple other instances that weren't escaping arbitrary strings. * src/conf/domain_conf.c (virDomainFSDefFormat) (virDomainGraphicsListenDefFormat, virDomainSnapshotDefFormat): Escape arbitrary strings. --- src/conf/domain_conf.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7645628..d8f1a54 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8836,8 +8836,8 @@ virDomainFSDefFormat(virBufferPtr buf, } } - virBufferAsprintf(buf, " <target dir='%s'/>\n", - def->dst); + virBufferEscapeString(buf, " <target dir='%s'/>\n", + def->dst); if (def->readonly) virBufferAddLit(buf, " <readonly/>\n"); @@ -9639,7 +9639,7 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf, if (def->network && (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) { - virBufferAsprintf(buf, " network='%s'", def->network); + virBufferEscapeString(buf, " network='%s'", def->network); } virBufferAddLit(buf, "/>\n"); @@ -11101,15 +11101,15 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, flags |= VIR_DOMAIN_XML_INACTIVE; virBufferAddLit(&buf, "<domainsnapshot>\n"); - virBufferAsprintf(&buf, " <name>%s</name>\n", def->name); + virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); if (def->description) - virBufferAsprintf(&buf, " <description>%s</description>\n", - def->description); + virBufferEscapeString(&buf, " <description>%s</description>\n", + def->description); virBufferAsprintf(&buf, " <state>%s</state>\n", virDomainStateTypeToString(def->state)); if (def->parent) { virBufferAddLit(&buf, " <parent>\n"); - virBufferAsprintf(&buf, " <name>%s</name>\n", def->parent); + virBufferEscapeString(&buf, " <name>%s</name>\n", def->parent); virBufferAddLit(&buf, " </parent>\n"); } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", -- 1.7.4.4

When reverting to a snapshot, the inactive domain configuration has to be rolled back to what it was at the time of the snapshot. Additionally, if the VM is active and the snapshot was active, this now adds a failure if the two configurations are ABI incompatible, rather than risking qemu confusion. A future patch will add a VIR_DOMAIN_SNAPSHOT_FORCE flag, which will be required for two risky code paths - reverting to an older snapshot that lacked full domain information, and reverting from running to a live snapshot that requires starting a new qemu process. Any reverting that stops a running vm is also a form of data loss (discarding the current running state to go back in time), but as that is what reversion usually implies, it is probably not worth requiring a force flag. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Copy out domain. (qemuDomainRevertToSnapshot): Perform ABI compatibility checks. --- src/qemu/qemu_driver.c | 77 +++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8fafade..df51d83 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8729,12 +8729,14 @@ cleanup: return ret; } -static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, - const char *xmlDesc, - unsigned int flags) +static virDomainSnapshotPtr +qemuDomainSnapshotCreateXML(virDomainPtr domain, + const char *xmlDesc, + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; + char *xml = NULL; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -8768,16 +8770,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, 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 (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, QEMU_EXPECTED_VIRT_TYPES, parse_flags))) @@ -8791,8 +8783,27 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, /* XXX Ensure ABI compatibility before replacing anything. */ virDomainSnapshotObjListRemove(&vm->snapshots, prior); } + } else { + /* Easiest way to clone inactive portion of vm->def is via + * conversion in and back out of xml. */ + if (!(xml = virDomainDefFormat(vm->def, (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE))) || + !(def->dom = virDomainDefParseString(driver->caps, xml, + QEMU_EXPECTED_VIRT_TYPES, + 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 (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; @@ -8855,6 +8866,7 @@ cleanup: virDomainObjUnlock(vm); } virDomainSnapshotDefFree(def); + VIR_FREE(xml); qemuDriverUnlock(driver); return snapshot; } @@ -9118,6 +9130,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, int detail; qemuDomainObjPrivatePtr priv; int rc; + virDomainDefPtr config = NULL; virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); @@ -9173,7 +9186,30 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * in the failure cases where we know there was no change? */ } + /* Prepare to copy the snapshot inactive xml as the config of this + * domain. Easiest way is by a round trip through xml. + * + * XXX Should domain snapshots track live xml rather + * than inactive xml? */ snap->def->current = true; + if (snap->def->dom) { + char *xml; + if (!(xml = virDomainDefFormat(snap->def->dom, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)))) + goto cleanup; + config = virDomainDefParseString(driver->caps, xml, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE); + VIR_FREE(xml); + if (!config) + goto cleanup; + } else { + /* XXX Fail if VIR_DOMAIN_REVERT_FORCE is not set, rather than + * blindly hoping for the best. */ + VIR_WARN("snapshot is lacking rollback information for domain '%s'", + snap->def->name); + } if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -9191,6 +9227,14 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * to have finer control. */ if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ + /* Check for ABI compatibility. */ + if (config && !virDomainDefCheckABIStability(vm->def, config)) { + /* XXX Add VIR_DOMAIN_REVERT_FORCE to permit killing + * and restarting a new qemu, since loadvm monitor + * command won't work. */ + goto endjob; + } + priv = vm->privateData; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { /* Transitions 5, 6 */ @@ -9220,9 +9264,14 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * failed loadvm attempt? */ goto endjob; } + if (config) + virDomainObjAssignDef(vm, config, false); } else { /* Transitions 2, 3 */ was_stopped = true; + if (config) + virDomainObjAssignDef(vm, config, false); + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, true, false, -1, NULL, snap, VIR_VM_OP_CREATE); @@ -9302,6 +9351,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } goto endjob; } + if (config) + virDomainObjAssignDef(vm, config, false); if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { -- 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 d8f1a54..36a8252 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; @@ -8638,8 +8662,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) { @@ -8713,6 +8742,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 54244be..1be7ed9 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; }; @@ -1693,6 +1704,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 94e28e4..9f6c784 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

Reverting to a state prior to an external snapshot risks corrupting any other branches in the snapshot hierarchy that were using the snapshot as a read-only backing file. So disk snapshot code will default to preventing reverting to a snapshot that has any children, meaning that deleting just the children of a snapshot becomes a useful operation in preparing that snapshot for being a future reversion target. The code for the new flag is simple - it's one less deletion, plus a tweak to keep the current snapshot correct. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY): New flag. * src/libvirt.c (virDomainSnapshotDelete): Document it, and enforce mutual exclusion. * src/qemu/qemu_driver.c (qemuDomainSnapshotDelete): Implement it. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 32 +++++++++++++++++++++++--------- src/qemu/qemu_driver.c | 25 +++++++++++++++++++++---- 3 files changed, 45 insertions(+), 13 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 4a5dbff..64441d9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2620,6 +2620,7 @@ int virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, typedef enum { VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = (1 << 0), /* Also delete children */ VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = (1 << 1), /* Delete just metadata */ + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY = (1 << 2), /* Delete just children */ } virDomainSnapshotDeleteFlags; int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, diff --git a/src/libvirt.c b/src/libvirt.c index 3ab2ea5..9b3d2b6 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16052,16 +16052,22 @@ error: * * Delete the snapshot. * - * If @flags is 0, then just this snapshot is deleted, and changes from - * this snapshot are automatically merged into children snapshots. If - * @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, then this snapshot - * and any children snapshots are deleted. If @flags includes - * VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, then any snapshot metadata - * tracked by libvirt is removed while keeping the snapshot contents - * intact; if a hypervisor does not require any libvirt metadata to - * track snapshots, then this flag is silently ignored. + * If @flags is 0, then just this snapshot is deleted, and changes + * from this snapshot are automatically merged into children + * snapshots. If @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, + * then this snapshot and any descendant snapshots are deleted. If + * @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, then any + * descendant snapshots are deleted, but this snapshot remains. These + * two flags are mutually exclusive. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, then + * any snapshot metadata tracked by libvirt is removed while keeping + * the snapshot contents intact; if a hypervisor does not require any + * libvirt metadata to track snapshots, then this flag is silently + * ignored. * - * Returns 0 if the snapshot was successfully deleted, -1 on error. + * Returns 0 if the selected snapshot(s) were successfully deleted, + * -1 on error. */ int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, @@ -16086,6 +16092,14 @@ virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto error; } + if ((flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) && + (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("children and children_only flags are " + "mutually exclusive")); + goto error; + } + if (conn->driver->domainSnapshotDelete) { int ret = conn->driver->domainSnapshotDelete(snapshot, flags); if (ret < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df51d83..3aefaef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9463,7 +9463,8 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, -1); qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); @@ -9485,7 +9486,8 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { rem.driver = driver; rem.vm = vm; rem.metadata_only = metadata_only; @@ -9497,8 +9499,20 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, &rem); if (rem.err < 0) goto endjob; - if (rem.current) + if (rem.current) { + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + snap->def->current = true; + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->snapshotDir) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to set snapshot '%s' as current"), + snap->def->name); + snap->def->current = false; + goto endjob; + } + } vm->current_snapshot = snap; + } } else { rep.driver = driver; rep.parent = snap->def->parent; @@ -9511,7 +9525,10 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto endjob; } - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) + ret = 0; + else + ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); endjob: if (qemuDomainObjEndJob(driver, vm) == 0) -- 1.7.4.4

Since a snapshot is fully recoverable, it is useful to have a snapshot as a means of hibernating a guest, then reverting to the snapshot to wake the guest up. This mode of usage is similar to 'virsh save/virsh restore', except that virsh save uses an external file while virsh snapshot keeps the vm state internal to a qcow2 file. However, it only works on persistent domains. In the usage pattern of snapshot/revert for hibernating a guest, there is no need to keep the guest running between the two points in time, especially since that would generate runtime state that would just be discarded. Add a flag to make it possible to stop the domain after the snapshot has completed. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_HALT): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotCreateActive): Implement it. --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 12 ++++++++++++ src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++---- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 64441d9..b35042c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2565,6 +2565,8 @@ typedef enum { snapshot current */ VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA = (1 << 2), /* Make snapshot without remembering it */ + VIR_DOMAIN_SNAPSHOT_CREATE_HALT = (1 << 3), /* Stop running guest + after snapshot */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt.c b/src/libvirt.c index 9b3d2b6..0ae6cc2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15632,6 +15632,12 @@ error: * the just-created snapshot has its metadata deleted. This flag is * incompatible with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_HALT, then the domain + * will be inactive after the snapshot completes, regardless of whether + * it was active before; otherwise, a running domain will still be + * running after the snapshot. This flag is invalid on transient domains, + * and is incompatible with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. + * * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure. */ virDomainSnapshotPtr @@ -15675,6 +15681,12 @@ virDomainSnapshotCreateXML(virDomainPtr domain, _("redefine and no metadata flags are mutually exclusive")); goto error; } + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("redefine and halt flags are mutually exclusive")); + goto error; + } if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3aefaef..d3968cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8678,7 +8678,8 @@ static int qemuDomainSnapshotCreateActive(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr *vmptr, - virDomainSnapshotObjPtr snap) + virDomainSnapshotObjPtr snap, + unsigned int flags) { virDomainObjPtr vm = *vmptr; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -8708,6 +8709,24 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); 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) && @@ -8719,7 +8738,7 @@ cleanup: _("resuming after snapshot failed")); } - if (qemuDomainObjEndJob(driver, vm) == 0) { + if (vm && 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; @@ -8746,7 +8765,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | - VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_CREATE_HALT, NULL); if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || @@ -8769,6 +8789,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, "%s", _("domain is marked for auto destroy")); goto cleanup; } + if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot halt after transient domain snapshot")); + goto cleanup; + } if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, QEMU_EXPECTED_VIRT_TYPES, @@ -8841,7 +8866,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } else { if (qemuDomainSnapshotCreateActive(domain->conn, driver, - &vm, snap) < 0) + &vm, snap, flags) < 0) goto cleanup; } -- 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 4ceb536..572cf3f 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 854ee7f..0e4bb72 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. For now, no driver accepts this element; that will come later. <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 | 117 +++++++++- docs/schemas/domainsnapshot.rng | 52 ++++ src/conf/domain_conf.c | 280 ++++++++++++++++++++++ src/conf/domain_conf.h | 25 ++- src/libvirt_private.syms | 1 + tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 16 ++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 42 ++++ 7 files changed, 526 insertions(+), 7 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 99ff4d4..9c7631c 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -64,9 +64,9 @@ <p> Attributes of libvirt snapshots are stored as child elements of the <code>domainsnapshot</code> element. At snapshot creation - time, normally only the <code>name</code> - and <code>description</code> elements are settable; the rest of - the fields are ignored on creation, and will be filled in by + time, normally only the <code>name</code>, <code>description</code>, + and <code>disks</code> elements are settable; the rest of the + fields are ignored on creation, and will be filled in by libvirt in for informational purposes by <code>virDomainSnapshotGetXMLDesc()</code>. However, when redefining a snapshot with @@ -102,6 +102,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. @@ -140,14 +192,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;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> + <code>virDomainSnapshotGetXMLDesc()</code>:</p> <pre> &lt;domainsnapshot&gt; &lt;name&gt;1270477159&lt;/name&gt; @@ -157,13 +216,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 6e9d822..aa0ad83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10989,18 +10989,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; +} + /* flags is bitwise-or of virDomainSnapshotParseFlags. * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then * caps and expectedVirtTypes are ignored. @@ -11015,6 +11079,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; @@ -11056,6 +11122,25 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { + 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); + } else { + virDomainReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("unable to handle disk requests in snapshot")); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { @@ -11123,6 +11208,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, cleanup: VIR_FREE(creation); VIR_FREE(state); + VIR_FREE(nodes); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -11131,12 +11217,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); @@ -11156,6 +11408,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 c92fc4b..2e34ace 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. */ @@ -1343,7 +1360,8 @@ struct _virDomainSnapshotObjList { typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, - VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 1, + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, @@ -1355,6 +1373,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 03819a4..1aa33ef 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 14ec429..3c9420e 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; + unsigned 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[] = { @@ -13052,6 +13132,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 c40e9f3..b00f49c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -521,6 +521,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

Expose the disk-only flag through virsh. Additionally, make virsh snapshot-create-as take an arbitrary number of diskspecs, which can be used to build up the xml for <domainsnapshot>. * tools/virsh.c (cmdSnapshotCreate): Add --disk-only. (cmdSnapshotCreateAs): Likewise, and add argv diskspec. (vshParseSnapshotDiskspec): New helper function. (vshCmddefGetOption): Allow naming of argv field. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document them. * tests/virsh-optparse: Test snapshot-create-as parsing. --- tests/virsh-optparse | 20 +++++++++++ tools/virsh.c | 88 +++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 28 ++++++++++++++-- 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 7b3a25d..cd5e3eb 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -67,6 +67,26 @@ for args in \ virsh -d0 -c $test_url setvcpus $args >out 2>>err || fail=1 LC_ALL=C sort out | compare - exp-out || fail=1 done + +# Another complex parsing example +cat <<\EOF > exp-out || framework_failure +<domainsnapshot> + <description>1&lt;2</description> + <disks> + <disk name='vda' snapshot='external'> + <source file='a&amp;b,c'/> + </disk> + <disk name='vdb'/> + </disks> +</domainsnapshot> + + +EOF +virsh -c $test_url snapshot-create-as --print-xml test \ + --diskspec 'vda,file=a&b,,c,snapshot=external' --description '1<2' \ + --diskspec vdb >out 2>>err || fail=1 +compare out exp-out || fail=1 + test -s err && fail=1 (exit $fail); exit $fail diff --git a/tools/virsh.c b/tools/virsh.c index 3c9420e..62e6d0f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12156,6 +12156,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"current", VSH_OT_BOOL, 0, N_("with redefine, set current snapshot")}, {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, {"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} }; @@ -12176,6 +12177,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; 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; @@ -12214,6 +12217,62 @@ cleanup: /* * "snapshot-create-as" command */ +static int +vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str) +{ + int ret = -1; + char *name = NULL; + char *snapshot = NULL; + char *driver = NULL; + char *file = NULL; + char *spec = vshStrdup(ctl, str); + char *tmp = spec; + size_t len = strlen(str); + + if (*str == ',') + goto cleanup; + name = tmp; + while ((tmp = strchr(tmp, ','))) { + if (tmp[1] == ',') { + /* Recognize ,, as an escape for a literal comma */ + memmove(&tmp[1], &tmp[2], len - (tmp - spec) + 2); + len--; + tmp++; + continue; + } + /* Terminate previous string, look for next recognized one */ + *tmp++ = '\0'; + if (!snapshot && STRPREFIX(tmp, "snapshot=")) + snapshot = tmp + strlen("snapshot="); + else if (!driver && STRPREFIX(tmp, "driver=")) + driver = tmp + strlen("driver="); + else if (!file && STRPREFIX(tmp, "file=")) + file = tmp + strlen("file="); + else + goto cleanup; + } + + virBufferEscapeString(buf, " <disk name='%s'", name); + if (snapshot) + virBufferAsprintf(buf, " snapshot='%s'", snapshot); + if (driver || file) { + virBufferAddLit(buf, ">\n"); + if (driver) + virBufferAsprintf(buf, " <driver type='%s'/>\n", driver); + if (file) + virBufferEscapeString(buf, " <source file='%s'/>\n", file); + virBufferAddLit(buf, " </disk>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + ret = 0; +cleanup: + if (ret < 0) + vshError(ctl, _("unable to parse diskspec: %s"), str); + VIR_FREE(spec); + return ret; +} + static const vshCmdInfo info_snapshot_create_as[] = { {"help", N_("Create a snapshot from a set of args")}, {"desc", N_("Create a snapshot (disk and RAM) from arguments")}, @@ -12227,6 +12286,9 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"print-xml", VSH_OT_BOOL, 0, N_("print XML document rather than create")}, {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, {"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")}, + {"diskspec", VSH_OT_ARGV, 0, + N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")}, {NULL, 0, 0, NULL} }; @@ -12240,11 +12302,14 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *desc = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned int flags = 0; + const vshCmdOpt *opt = NULL; if (vshCommandOptBool(cmd, "no-metadata")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; 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; @@ -12264,6 +12329,16 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virBufferEscapeString(&buf, " <name>%s</name>\n", name); if (desc) virBufferEscapeString(&buf, " <description>%s</description>\n", desc); + if (vshCommandOptBool(cmd, "diskspec")) { + virBufferAddLit(&buf, " <disks>\n"); + while ((opt = vshCommandOptArgv(cmd, opt))) { + if (vshParseSnapshotDiskspec(ctl, &buf, opt->data) < 0) { + virBufferFreeAndReset(&buf); + goto cleanup; + } + } + virBufferAddLit(&buf, " </disks>\n"); + } virBufferAddLit(&buf, "</domainsnapshot>\n"); buffer = virBufferContentAndReset(&buf); @@ -12273,11 +12348,6 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptBool(cmd, "print-xml")) { - if (vshCommandOptBool(cmd, "halt")) { - vshError(ctl, "%s", - _("--print-xml and --halt are mutually exclusive")); - goto cleanup; - } vshPrint(ctl, "%s\n", buffer); ret = true; goto cleanup; @@ -13425,12 +13495,8 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, vshError(ctl, _("option --%s already seen"), name); return NULL; } - if (opt->type == VSH_OT_ARGV) { - vshError(ctl, _("variable argument <%s> " - "should not be used with --<%s>"), name, name); - return NULL; - } - *opts_seen |= 1 << i; + if (opt->type != VSH_OT_ARGV) + *opts_seen |= 1 << i; return opt; } } diff --git a/tools/virsh.pod b/tools/virsh.pod index b00f49c..5d345af 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1706,11 +1706,12 @@ used to represent properties of snapshots. =over 4 =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] -| [I<--no-metadata>] [I<--halt>]} +| [I<--no-metadata>] [I<--halt>] [I<--disk-only>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot -are the <name> and <description> elements; the rest of the fields are +are the <name> and <description> elements, 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. The new snapshot will become current, as listed by B<snapshot-current>. @@ -1718,6 +1719,14 @@ The new snapshot will become current, as listed by B<snapshot-current>. If I<--halt> is specified, the domain will be left in an inactive state after the snapshot is created. +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. + If I<--redefine> is specified, then all XML elements produced by B<snapshot-dumpxml> are valid; this can be used to migrate snapshot hierarchy from one machine to another, to recreate hierarchy for the @@ -1741,13 +1750,26 @@ by command such as B<destroy> or by internal guest action). =item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>] [I<--halt>]} [I<name>] [I<description>] +[I<--disk-only> [I<diskspec>]...] 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. + +The I<--disk-only> flag is used to request a disk-only snapshot. When +this flag is in use, the command can also take additional I<diskspec> +arguments to add <disk> elements to the xml. Each <diskspec> is in the +form B<disk[,snapshot=type][,driver=type][,file=name]>. To include a +literal comma in B<disk> or in B<file=name>, escape it with a second +comma. For example, a diskspec of "vda,snapshot=external,file=/path/to,,new" +results in the following XML: + <disk name='vda' snapshot='external'> + <source file='/path/to,new'/> + </disk> If I<--no-metadata> is specified, then the snapshot data is created, but any metadata is immediately discarded (that is, libvirt does not -- 1.7.4.4

Snapshots alter the set of disk image files opened by qemu, so they must be audited. But they don't involve a full disk definition structure, just the new filename. Make the next patch easier by refactoring the audit routines to just operate on file name. * src/conf/domain_audit.h (virDomainAuditDisk): Update prototype. * src/conf/domain_audit.c (virDomainAuditDisk): Act on strings, not definition structures. (virDomainAuditStart): Update caller. * src/qemu/qemu_hotplug.c (qemuDomainChangeEjectableMedia) (qemuDomainAttachPciDiskDevice, qemuDomainAttachSCSIDisk) (qemuDomainAttachUsbMassstorageDevice) (qemuDomainDetachPciDiskDevice, qemuDomainDetachDiskDevice): Likewise. --- src/conf/domain_audit.c | 12 ++++-------- src/conf/domain_audit.h | 4 ++-- src/qemu/qemu_hotplug.c | 18 +++++++++--------- 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 9d89c94..7d3eb5b 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -60,7 +60,7 @@ virDomainAuditGetRdev(const char *path ATTRIBUTE_UNUSED) void virDomainAuditDisk(virDomainObjPtr vm, - virDomainDiskDefPtr oldDef, virDomainDiskDefPtr newDef, + const char *oldDef, const char *newDef, const char *reason, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -80,15 +80,11 @@ virDomainAuditDisk(virDomainObjPtr vm, virt = "?"; } - if (!(oldsrc = virAuditEncode("old-disk", - oldDef && oldDef->src ? - oldDef->src : "?"))) { + if (!(oldsrc = virAuditEncode("old-disk", VIR_AUDIT_STR(oldDef)))) { VIR_WARN("OOM while encoding audit message"); goto cleanup; } - if (!(newsrc = virAuditEncode("new-disk", - newDef && newDef->src ? - newDef->src : "?"))) { + if (!(newsrc = virAuditEncode("new-disk", VIR_AUDIT_STR(newDef)))) { VIR_WARN("OOM while encoding audit message"); goto cleanup; } @@ -520,7 +516,7 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) for (i = 0 ; i < vm->def->ndisks ; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; if (disk->src) /* Skips CDROM without media initially inserted */ - virDomainAuditDisk(vm, NULL, disk, "start", true); + virDomainAuditDisk(vm, NULL, disk->src, "start", true); } for (i = 0 ; i < vm->def->nfss ; i++) { diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 0e88fd3..da9e257 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -35,8 +35,8 @@ void virDomainAuditStop(virDomainObjPtr vm, const char *reason) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainAuditDisk(virDomainObjPtr vm, - virDomainDiskDefPtr oldDef, - virDomainDiskDefPtr newDef, + const char *oldDef, + const char *newDef, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7fdfa0..cd59283 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -114,7 +114,7 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, origdisk, disk, "update", ret >= 0); + virDomainAuditDisk(vm, origdisk->src, disk->src, "update", ret >= 0); if (ret < 0) goto error; @@ -224,7 +224,7 @@ int qemuDomainAttachPciDiskDevice(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, NULL, disk, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -468,7 +468,7 @@ int qemuDomainAttachSCSIDisk(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, NULL, disk, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -560,7 +560,7 @@ int qemuDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, } qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, NULL, disk, "attach", ret >= 0); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); if (ret < 0) goto error; @@ -1277,14 +1277,14 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, detach, NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } else { if (qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, detach, NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } } @@ -1294,7 +1294,7 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, detach, NULL, "detach", true); + virDomainAuditDisk(vm, detach->src, NULL, "detach", true); if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, @@ -1372,7 +1372,7 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, detach, NULL, "detach", false); + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); goto cleanup; } @@ -1381,7 +1381,7 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver, qemuDomainObjExitMonitorWithDriver(driver, vm); - virDomainAuditDisk(vm, detach, NULL, "detach", true); + virDomainAuditDisk(vm, detach->src, NULL, "detach", true); virDomainDiskRemove(vm->def, i); -- 1.7.4.4

With this, it is now possible to create external snapshots even when SELinux is enforcing, and to protect the new file with a lock manager. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive): Create and register new file with proper permissions and locks. (qemuDomainSnapshotCreateDiskActive): Update caller. --- src/qemu/qemu_driver.c | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4862c95..09d6542 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -87,6 +87,7 @@ #include "configmake.h" #include "threadpool.h" #include "locking/lock_manager.h" +#include "locking/domain_lock.h" #include "virkeycode.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -8839,7 +8840,8 @@ cleanup: /* The domain is expected to hold monitor lock. */ static int -qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, +qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, + virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk) { @@ -8848,6 +8850,10 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, char *source = NULL; char *driverType = NULL; int ret = -1; + int fd = -1; + char *origsrc = NULL; + char *origdriver = NULL; + bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -8863,7 +8869,35 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, goto cleanup; } - /* XXX create new file and set selinux labels */ + /* create the stub file and set selinux labels; manipulate disk in + * place, in a way that can be reverted on failure. */ + fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + &need_unlink, NULL); + if (fd < 0) + goto cleanup; + VIR_FORCE_CLOSE(fd); + + origsrc = disk->src; + disk->src = source; + origdriver = disk->driverType; + disk->driverType = driverType; + + if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + goto cleanup; + if (virSecurityManagerSetImageLabel(driver->securityManager, vm, + disk) < 0) { + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + VIR_WARN("Unable to release lock on %s", source); + goto cleanup; + } + need_unlink = false; + + disk->src = origsrc; + origsrc = NULL; + disk->driverType = origdriver; + origdriver = NULL; + + /* create the actual snapshot */ ret = qemuMonitorDiskSnapshot(priv->mon, device, source); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) @@ -8883,6 +8917,12 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, * configuration changes awaiting the next boot? */ cleanup: + if (origsrc) { + disk->src = origsrc; + disk->driverType = origdriver; + } + if (need_unlink && unlink(source)) + VIR_WARN("unable to unlink just-created %s", source); VIR_FREE(device); VIR_FREE(source); VIR_FREE(driverType); @@ -8934,7 +8974,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) continue; - ret = qemuDomainSnapshotCreateSingleDiskActive(vm, + ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], vm->def->disks[i]); if (ret < 0) -- 1.7.4.4