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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list