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

[Cole Robinson]

On 09/27/2013 02:13 AM, Michal Privoznik wrote:
> 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.
> 

I restored the default from qemu_driver.c and pushed now.

Thanks,
Cole