6:48 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org