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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121107/d314da6b/attachment-0001.sig>