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

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.