[libvirt] [PATCH 2/3] qemu: Refactor qemuDomainSnapshotCreateXML

[Daniel P. Berrange]

On Fri, Feb 25, 2011 at 07:04:10PM +0100, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_driver.c |  125 +++++++++++++++++++++++++++++------------------
>  1 files changed, 77 insertions(+), 48 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7fc08e8..ba046d7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5987,6 +5987,81 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
>      return 1;
>  }
>  
> +/* The domain is expected to be locked and inactive. */
> +static int
> +qemuDomainSnapshotCreateInactive(virDomainObjPtr vm,
> +                                 virDomainSnapshotObjPtr snap)
> +{
> +    const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL };
> +    int ret = -1;
> +    int i;
> +
> +    qemuimgarg[0] = qemuFindQemuImgBinary();
> +    if (qemuimgarg[0] == NULL) {
> +        /* qemuFindQemuImgBinary set the error */
> +        goto cleanup;
> +    }
> +
> +    qemuimgarg[3] = snap->def->name;
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        /* FIXME: we also need to handle LVM here */
> +        /* FIXME: if we fail halfway through this loop, we are in an
> +         * inconsistent state.  I'm not quite sure what to do about that
> +         */
> +        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> +            if (!vm->def->disks[i]->driverType ||
> +                STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
> +                qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                                _("Disk device '%s' does not support"
> +                                  " snapshotting"),
> +                                vm->def->disks[i]->info.alias);
> +                goto cleanup;
> +            }
> +
> +            qemuimgarg[4] = vm->def->disks[i]->src;
> +
> +            if (virRun(qemuimgarg, NULL) < 0) {
> +                virReportSystemError(errno,
> +                                     _("Failed to run '%s' to create snapshot"
> +                                       " '%s' from disk '%s'"),
> +                                     qemuimgarg[0], snap->def->name,
> +                                     vm->def->disks[i]->src);
> +                goto cleanup;
> +            }
> +        }
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(qemuimgarg[0]);
> +    return ret;
> +}
> +
> +/* The domain is expected to be locked and active. */
> +static int
> +qemuDomainSnapshotCreateActive(struct qemud_driver *driver,
> +                               virDomainObjPtr *vmptr,
> +                               virDomainSnapshotObjPtr snap)
> +{
> +    virDomainObjPtr vm = *vmptr;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret;
> +
> +    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> +        return -1;
> +
> +    qemuDomainObjEnterMonitorWithDriver(driver, vm);
> +    ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
> +    qemuDomainObjExitMonitorWithDriver(driver, vm);
> +
> +    if (qemuDomainObjEndJob(vm) == 0)
> +        *vmptr = NULL;
> +
> +    return ret;
> +}
> +
>  static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                                                          const char *xmlDesc,
>                                                          unsigned int flags)
> @@ -5997,8 +6072,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>      virDomainSnapshotPtr snapshot = NULL;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      virDomainSnapshotDefPtr def;
> -    const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL };
> -    int i;
>  
>      virCheckFlags(0, NULL);
>  
> @@ -6029,54 +6102,11 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>  
>      /* actually do the snapshot */
>      if (!virDomainObjIsActive(vm)) {
> -        qemuimgarg[0] = qemuFindQemuImgBinary();
> -        if (qemuimgarg[0] == NULL)
> -            /* qemuFindQemuImgBinary set the error */
> +        if (qemuDomainSnapshotCreateInactive(vm, snap) < 0)
>              goto cleanup;
> -
> -        qemuimgarg[3] = snap->def->name;
> -
> -        for (i = 0; i < vm->def->ndisks; i++) {
> -            /* FIXME: we also need to handle LVM here */
> -            /* FIXME: if we fail halfway through this loop, we are in an
> -             * inconsistent state.  I'm not quite sure what to do about that
> -             */
> -            if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> -                if (!vm->def->disks[i]->driverType ||
> -                    STRNEQ(vm->def->disks[i]->driverType, "qcow2")) {
> -                    qemuReportError(VIR_ERR_OPERATION_INVALID,
> -                                    _("Disk device '%s' does not support snapshotting"),
> -                                    vm->def->disks[i]->info.alias);
> -                    goto cleanup;
> -                }
> -
> -                qemuimgarg[4] = vm->def->disks[i]->src;
> -
> -                if (virRun(qemuimgarg, NULL) < 0) {
> -                    virReportSystemError(errno,
> -                                         _("Failed to run '%s' to create snapshot '%s' from disk '%s'"),
> -                                         qemuimgarg[0], snap->def->name,
> -                                         vm->def->disks[i]->src);
> -                    goto cleanup;
> -                }
> -            }
> -        }
>      }
>      else {
> -        qemuDomainObjPrivatePtr priv;
> -        int ret;
> -
> -        if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> -            goto cleanup;
> -        priv = vm->privateData;
> -        qemuDomainObjEnterMonitorWithDriver(driver, vm);
> -        ret = qemuMonitorCreateSnapshot(priv->mon, def->name);
> -        qemuDomainObjExitMonitorWithDriver(driver, vm);
> -        if (qemuDomainObjEndJob(vm) == 0) {
> -            vm = NULL;
> -            goto cleanup;
> -        }
> -        if (ret < 0)
> +        if (qemuDomainSnapshotCreateActive(driver, &vm, snap) < 0)
>              goto cleanup;
>      }
>  
> @@ -6106,7 +6136,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>      snapshot = virGetDomainSnapshot(domain, snap->def->name);
>  
>  cleanup:
> -    VIR_FREE(qemuimgarg[0]);
>      if (vm)
>          virDomainObjUnlock(vm);
>      qemuDriverUnlock(driver);

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|