Re: [libvirt] [PATCHv3] snapshot: qemu: Add support for external inactive snapshots

8 Nov 2012

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

    