[libvirt] [PATCH v3 6/8] qemu: snapshot: Break out redefine preparation to shared function

[Michal Privoznik]

On 25.09.2013 21:15, Cole Robinson wrote:
> ---
>  src/conf/snapshot_conf.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/snapshot_conf.h |   7 +++
>  src/libvirt_private.syms |   1 +
>  src/qemu/qemu_driver.c   | 131 +----------------------------------------
>  4 files changed, 160 insertions(+), 129 deletions(-)

Almost.

> 
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 207a8fe..1fcc4cb 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1099,3 +1099,153 @@ virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap)
>  {
>      return virDomainSnapshotDefIsExternal(snap->def);
>  }
> +
> +int
> +virDomainSnapshotRedefinePrep(virDomainPtr domain,
> +                              virDomainObjPtr vm,
> +                              virDomainSnapshotDefPtr *defptr,
> +                              virDomainSnapshotObjPtr *snap,
> +                              bool *update_current,
> +                              unsigned int flags)
> +{
> +    virDomainSnapshotDefPtr def = *defptr;
> +    int ret = -1;
> +    int align_location;
> +    int align_match;

These two ^^^ had a default set ...

> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    virDomainSnapshotObjPtr other;
> +
> +    virUUIDFormat(domain->uuid, uuidstr);
> +
> +    /* Prevent circular chains */
> +    if (def->parent) {
> +        if (STREQ(def->name, def->parent)) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("cannot set snapshot %s as its own parent"),
> +                           def->name);
> +            goto cleanup;
> +        }
> +        other = virDomainSnapshotFindByName(vm->snapshots, def->parent);
> +        if (!other) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("parent %s for snapshot %s not found"),
> +                           def->parent, def->name);
> +            goto cleanup;
> +        }
> +        while (other->def->parent) {
> +            if (STREQ(other->def->parent, def->name)) {
> +                virReportError(VIR_ERR_INVALID_ARG,
> +                               _("parent %s would create cycle to %s"),
> +                               other->def->name, def->name);
> +                goto cleanup;
> +            }
> +            other = virDomainSnapshotFindByName(vm->snapshots,
> +                                                other->def->parent);
> +            if (!other) {
> +                VIR_WARN("snapshots are inconsistent for %s",
> +                         vm->def->name);
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* Check that any replacement is compatible */
> +    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
> +        def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("disk-only flag for snapshot %s requires "
> +                         "disk-snapshot state"),
> +                       def->name);
> +        goto cleanup;
> +
> +    }
> +
> +    if (def->dom &&
> +        memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("definition for snapshot %s must use uuid %s"),
> +                       def->name, uuidstr);
> +        goto cleanup;
> +    }
> +
> +    other = virDomainSnapshotFindByName(vm->snapshots, def->name);
> +    if (other) {
> +        if ((other->def->state == VIR_DOMAIN_RUNNING ||
> +             other->def->state == VIR_DOMAIN_PAUSED) !=
> +            (def->state == VIR_DOMAIN_RUNNING ||
> +             def->state == VIR_DOMAIN_PAUSED)) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("cannot change between online and offline "
> +                             "snapshot state in snapshot %s"),
> +                           def->name);
> +            goto cleanup;
> +        }
> +
> +        if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) !=
> +            (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("cannot change between disk snapshot and "
> +                             "system checkpoint in snapshot %s"),
> +                           def->name);
> +            goto cleanup;
> +        }
> +
> +        if (other->def->dom) {
> +            if (def->dom) {
> +                if (!virDomainDefCheckABIStability(other->def->dom,
> +                                                   def->dom))
> +                    goto cleanup;
> +            } else {
> +                /* Transfer the domain def */
> +                def->dom = other->def->dom;
> +                other->def->dom = NULL;
> +            }
> +        }
> +
> +        if (def->dom) {
> +            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
> +                virDomainSnapshotDefIsExternal(def)) {
> +                align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> +                align_match = false;
> +            }
> +
> +            if (virDomainSnapshotAlignDisks(def, align_location,
> +                                            align_match) < 0) {

... and here you're calling them with a random value (unless the
previous if evaluates true).

> +                /* revert stealing of the snapshot domain definition */
> +                if (def->dom && !other->def->dom) {
> +                    other->def->dom = def->dom;
> +                    def->dom = NULL;
> +                }
> +                goto cleanup;
> +            }
> +        }

ACK once you fix it.

Michal