[libvirt] [PATCHv3] snapshot: qemu: Add support for external inactive snapshots
This patch adds support for external disk snapshots of inactive domains. The snapshot is created by calling using qemu-img by calling: qemu-img create -f format_of_snapshot -o backing_file=/path/to/src,backing_fmt=format_of_backing_image /path/to/snapshot in case the backing image format is known or probing is allowed and otherwise: qemu-img create -f format_of_snapshot -o backing_file=/path/to/src /path/to/snapshot on each of the disks selected for snapshotting. This patch also modifies the snapshot preparing function to support creating external snapshots and to sanitize arguments. For now the user isn't able to mix external and internal snapshots but this restriction might be lifted in the future. --- Diff to v2: incorporated a ton of review feedback - fix of unlinking images on OOM - format command line arguments better - fix preparing function to reject flags properly --- src/qemu/qemu_driver.c | 219 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 179 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 01ba7eb..17de98e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10658,13 +10658,125 @@ qemuDomainSnapshotFSThaw(struct qemud_driver *driver, /* The domain is expected to be locked and inactive. */ static int -qemuDomainSnapshotCreateInactive(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap) +qemuDomainSnapshotCreateInactiveInternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) { return qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-c", false); } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool reuse) +{ + int i; + virDomainSnapshotDiskDefPtr snapdisk; + virDomainDiskDefPtr defdisk; + virCommandPtr cmd = NULL; + const char *qemuImgPath; + struct stat st; + + int ret = -1; + + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) + return -1; + + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = snap->def->dom->disks[snapdisk->index]; + + /* no-op if reuse is true and file exists and is valid */ + if (reuse) { + if (stat(snapdisk->file, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat snapshot image %s"), + snapdisk->file); + goto cleanup; + } + } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) { + /* the existing image is reused */ + continue; + } + } + + if (!snapdisk->format) + snapdisk->format = VIR_STORAGE_FILE_QCOW2; + + /* creates cmd line args: qemu-img create -f qcow2 -o */ + if (!(cmd = virCommandNewArgList(qemuImgPath, + "create", + "-f", + virStorageFileFormatTypeToString(snapdisk->format), + "-o", + NULL))) + goto cleanup; + + if (defdisk->format > 0) { + /* adds cmd line arg: backing_file=/path/to/backing/file,backing_fmd=format */ + virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s", + defdisk->src, + virStorageFileFormatTypeToString(defdisk->format)); + } else { + if (!driver->allowDiskFormatProbing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown image format of '%s' and " + "format probing is disabled"), + defdisk->src); + goto cleanup; + } + + /* adds cmd line arg: backing_file=/path/to/backing/file */ + virCommandAddArgFormat(cmd, "backing_file=%s", defdisk->src); + } + + /* adds cmd line args: /path/to/target/file */ + virCommandAddArg(cmd, snapdisk->file); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + virCommandFree(cmd); + cmd = NULL; + } + + /* update disk definitions */ + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = vm->def->disks[snapdisk->index]; + + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + VIR_FREE(defdisk->src); + if (!(defdisk->src = strdup(snapdisk->file))) { + /* we cannot rollback here in a sane way */ + virReportOOMError(); + return -1; + } + defdisk->format = snapdisk->format; + } + } + + ret = 0; + +cleanup: + virCommandFree(cmd); + + /* unlink images if creation has failed */ + if (ret < 0 && i > 0) { + for (; i > 0; i--) { + snapdisk = &(snap->def->disks[i]); + if (unlink(snapdisk->file) < 0) + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->file); + } + } + + return ret; +} + /* The domain is expected to be locked and active. */ static int @@ -10758,11 +10870,11 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, { int ret = -1; int i; - bool found = false; bool active = virDomainObjIsActive(vm); struct stat st; bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + bool found_internal = false; int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -10783,7 +10895,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && (dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG || dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) { - found = true; break; } if (vm->def->disks[i]->format > 0 && @@ -10803,7 +10914,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name); goto cleanup; } - found = true; + found_internal = true; break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: @@ -10837,7 +10948,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name, disk->file); goto cleanup; } - found = true; external++; break; @@ -10852,15 +10962,37 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, } } - /* external snapshot is possible without specifying a disk to snapshot */ - if (!found && - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + /* internal snapshot requires a disk image to store the memory image to */ + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && + !found_internal) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("internal checkpoints require at least " + "one disk to be selected for snapshot")); + goto cleanup; + } + + /* disk snapshot requires at least one disk */ + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("internal and disk-only snapshots require at least " + _("disk-only snapshots require at least " "one disk to be selected for snapshot")); goto cleanup; } + /* For now, we don't allow mixing internal and external disks. + * XXX technically, we could mix internal and external disks for + * offline snapshots */ + if (found_internal && external) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("mixing internal and external snapshots is not " + "supported yet")); + goto cleanup; + } + + /* Alter flags to let later users know what we learned. */ + if (external && !active) + *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active) { if (external == 1 || qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { @@ -11360,6 +11492,25 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup; + /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && + (!virDomainObjIsActive(vm) || + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("live snapshot creation is supported only " + "with external checkpoints")); + goto cleanup; + } + if ((def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) && + flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("disk-only snapshot creation is not compatible with " + "memory snapshot")); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { /* Prevent circular chains */ if (def->parent) { @@ -11472,15 +11623,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots of inactive domains not " - "implemented yet")); - goto cleanup; - } align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; - def->state = VIR_DOMAIN_DISK_SNAPSHOT; + if (virDomainObjIsActive(vm)) + def->state = VIR_DOMAIN_DISK_SNAPSHOT; + else + def->state = VIR_DOMAIN_SHUTOFF; def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); @@ -11523,25 +11671,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } } - /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && - (!virDomainObjIsActive(vm) || - snap->def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("live snapshot creation is supported only " - "with external checkpoints")); - goto cleanup; - } - if ((snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) && - flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("disk-only snapshot creation is not compatible with " - "memory snapshot")); - goto cleanup; - } - /* actually do the snapshot */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { /* XXX Should we validate that the redefined snapshot even @@ -11561,9 +11690,19 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } } else { - /* inactive */ - if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) - goto cleanup; + /* inactive; qemuDomainSnapshotPrepare guaranteed that we + * aren't mixing internal and external, and altered flags to + * contain DISK_ONLY if there is an external disk. */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + bool reuse = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); + + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, + reuse) < 0) + goto cleanup; + } else { + if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0) + goto cleanup; + } } /* If we fail after this point, there's not a whole lot we can -- 1.8.0
On 11/07/2012 04:06 PM, Peter Krempa wrote:
This patch adds support for external disk snapshots of inactive domains. The snapshot is created by calling using qemu-img by calling:
qemu-img create -f format_of_snapshot -o backing_file=/path/to/src,backing_fmt=format_of_backing_image /path/to/snapshot
+qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool reuse)
+ /* no-op if reuse is true and file exists and is valid */ + if (reuse) { + if (stat(snapdisk->file, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat snapshot image %s"), + snapdisk->file); + goto cleanup; + } + } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) { + /* the existing image is reused */ + continue;
Hey, I just realized that the logic I want here (if reuse, then check that it exists; if !reuse, then check that we aren't nuking unrelated data) was already run during qemuDomainSnapshotPrepare. So we can simplify this - if we got here, then we already know that the file exists and is ready to reuse, so we can simplify this code.
+ + /* update disk definitions */ + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = vm->def->disks[snapdisk->index]; + + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + VIR_FREE(defdisk->src); + if (!(defdisk->src = strdup(snapdisk->file))) { + /* we cannot rollback here in a sane way */ + virReportOOMError();
True, but OOM is enough of a corner case that I'm not too bothered by this.
+ return -1; + } + defdisk->format = snapdisk->format; + } + } + + ret = 0; + +cleanup: + virCommandFree(cmd); + + /* unlink images if creation has failed */ + if (ret < 0 && i > 0) { + for (; i > 0; i--) { + snapdisk = &(snap->def->disks[i]); + if (unlink(snapdisk->file) < 0)
Possible NULL deref, if you have a disk with no snapshot earlier in the array than a disk with an external snapshot. Also, we don't want to unlink() pre-existing block devices, so it may make more sense to pay attention to whether we actually created a file. ACK with this squashed in: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 17de98e..cea1c2f 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -10677,31 +10677,26 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, virDomainDiskDefPtr defdisk; virCommandPtr cmd = NULL; const char *qemuImgPath; - struct stat st; + virBitmapPtr created; int ret = -1; if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) return -1; - for (i = 0; i < snap->def->ndisks; i++) { + if (!(created = virBitmapNew(snap->def->ndisks))) { + virReportOOMError(); + return -1; + } + + /* If reuse is true, then qemuDomainSnapshotPrepare already + * ensured that the new files exist, and it was up to the user to + * create them correctly. */ + for (i = 0; i < snap->def->ndisks && !reuse; i++) { snapdisk = &(snap->def->disks[i]); defdisk = snap->def->dom->disks[snapdisk->index]; - - /* no-op if reuse is true and file exists and is valid */ - if (reuse) { - if (stat(snapdisk->file, &st) < 0) { - if (errno != ENOENT) { - virReportSystemError(errno, - _("unable to stat snapshot image %s"), - snapdisk->file); - goto cleanup; - } - } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) { - /* the existing image is reused */ - continue; - } - } + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; if (!snapdisk->format) snapdisk->format = VIR_STORAGE_FILE_QCOW2; @@ -10736,6 +10731,9 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, /* adds cmd line args: /path/to/target/file */ virCommandAddArg(cmd, snapdisk->file); + if (!virFileExists(snapdisk->file)) + ignore_value(virBitmapSetBit(created, i)); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -10753,7 +10751,8 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, if (!(defdisk->src = strdup(snapdisk->file))) { /* we cannot rollback here in a sane way */ virReportOOMError(); - return -1; + i = snap->def->ndisks; + goto cleanup; } defdisk->format = snapdisk->format; } @@ -10765,14 +10764,16 @@ cleanup: virCommandFree(cmd); /* unlink images if creation has failed */ - if (ret < 0 && i > 0) { - for (; i > 0; i--) { - snapdisk = &(snap->def->disks[i]); + if (ret < 0) { + ssize_t bit = -1; + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + snapdisk = &(snap->def->disks[bit]); if (unlink(snapdisk->file) < 0) VIR_WARN("Failed to remove snapshot image '%s'", snapdisk->file); } } + virBitmapFree(created); return ret; } struct stat st; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/08/12 01:48, Eric Blake wrote:
On 11/07/2012 04:06 PM, Peter Krempa wrote:
This patch adds support for external disk snapshots of inactive domains. The snapshot is created by calling using qemu-img by calling:
qemu-img create -f format_of_snapshot -o backing_file=/path/to/src,backing_fmt=format_of_backing_image /path/to/snapshot
+qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool reuse)
+ /* no-op if reuse is true and file exists and is valid */ + if (reuse) { + if (stat(snapdisk->file, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat snapshot image %s"), + snapdisk->file); + goto cleanup; + } + } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) { + /* the existing image is reused */ + continue;
Hey, I just realized that the logic I want here (if reuse, then check that it exists; if !reuse, then check that we aren't nuking unrelated data) was already run during qemuDomainSnapshotPrepare. So we can simplify this - if we got here, then we already know that the file exists and is ready to reuse, so we can simplify this code.
Yes, looking on the code now again, this was partly done in that way in the first version (apart from different reuse flag handling). I originally wanted the reuse flag to nuke existing files when re-creating the image files when reverting to a snapshot. After seeing your comment to that patch, I'll embed the logic to the revert function and will take into account snapshots on physical partitions that we can't just unlink.
+ + /* update disk definitions */ + for (i = 0; i < snap->def->ndisks; i++) { + snapdisk = &(snap->def->disks[i]); + defdisk = vm->def->disks[snapdisk->index]; + + if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + VIR_FREE(defdisk->src); + if (!(defdisk->src = strdup(snapdisk->file))) { + /* we cannot rollback here in a sane way */ + virReportOOMError();
True, but OOM is enough of a corner case that I'm not too bothered by this.
+ return -1; + } + defdisk->format = snapdisk->format; + } + } + + ret = 0; + +cleanup: + virCommandFree(cmd); + + /* unlink images if creation has failed */ + if (ret < 0 && i > 0) { + for (; i > 0; i--) { + snapdisk = &(snap->def->disks[i]); + if (unlink(snapdisk->file) < 0)
Possible NULL deref, if you have a disk with no snapshot earlier in the array than a disk with an external snapshot. Also, we don't want to unlink() pre-existing block devices, so it may make more sense to pay attention to whether we actually created a file.
ACK with this squashed in:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 17de98e..cea1c2f 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -10677,31 +10677,26 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, virDomainDiskDefPtr defdisk; virCommandPtr cmd = NULL; const char *qemuImgPath; - struct stat st; + virBitmapPtr created;
int ret = -1;
if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) return -1;
- for (i = 0; i < snap->def->ndisks; i++) { + if (!(created = virBitmapNew(snap->def->ndisks))) { + virReportOOMError(); + return -1; + } + + /* If reuse is true, then qemuDomainSnapshotPrepare already + * ensured that the new files exist, and it was up to the user to + * create them correctly. */ + for (i = 0; i < snap->def->ndisks && !reuse; i++) { snapdisk = &(snap->def->disks[i]); defdisk = snap->def->dom->disks[snapdisk->index]; - - /* no-op if reuse is true and file exists and is valid */ - if (reuse) { - if (stat(snapdisk->file, &st) < 0) { - if (errno != ENOENT) { - virReportSystemError(errno, - _("unable to stat snapshot image %s"), - snapdisk->file); - goto cleanup; - } - } else if (!S_ISBLK(st.st_mode) && st.st_size > 0) { - /* the existing image is reused */ - continue; - } - } + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue;
if (!snapdisk->format) snapdisk->format = VIR_STORAGE_FILE_QCOW2; @@ -10736,6 +10731,9 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, /* adds cmd line args: /path/to/target/file */ virCommandAddArg(cmd, snapdisk->file);
+ if (!virFileExists(snapdisk->file))
I added a comment here that explains what we're actually remembering here.
+ ignore_value(virBitmapSetBit(created, i)); + if (virCommandRun(cmd, NULL) < 0) goto cleanup;
@@ -10753,7 +10751,8 @@ qemuDomainSnapshotCreateInactiveExternal(struct qemud_driver *driver, if (!(defdisk->src = strdup(snapdisk->file))) { /* we cannot rollback here in a sane way */ virReportOOMError(); - return -1; + i = snap->def->ndisks;
This assignment is now dead code assuming ...
+ goto cleanup; } defdisk->format = snapdisk->format; } @@ -10765,14 +10764,16 @@ cleanup: virCommandFree(cmd);
/* unlink images if creation has failed */ - if (ret < 0 && i > 0) { - for (; i > 0; i--) { - snapdisk = &(snap->def->disks[i]); + if (ret < 0) { + ssize_t bit = -1; + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) {
... this new rollback mechanism.
+ snapdisk = &(snap->def->disks[bit]); if (unlink(snapdisk->file) < 0) VIR_WARN("Failed to remove snapshot image '%s'", snapdisk->file); } } + virBitmapFree(created);
return ret; } struct stat st;
Thanks for the review. I pushed the patch with your changes squashed in and fixed the two places I pointed out. Peter
participants (2)
-
Eric Blake -
Peter Krempa