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

[Peter Krempa]

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