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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list