Due to historical back-compat, 'virsh snapshot-create-as' favors internal snapshots (but can't be used on domains with raw storage), while 'virsh snapshot-create-as --disk-only) favors external snapshots. What's more, snapshots created with --disk-only while the domain was running are marked as snapshot state 'disk-snapshot', while snapshots created while the domain was offline are marked as snapshot state 'shutdown' (a 'disk-snapshot' image might not be quiescent, while a 'shutdown' snapshot always is). But this leads to some interesting problems: if we create a --disk-only snapshot of an offline guest, and then immediately try to 'virsh snapshot-create --redefine' using the resulting XML to overwrite the existing snapashot in place, things silently succeed, but 'virsh snapshot-create --redefine --disk-only' fails because the snapshot state is not 'disk-only'. Worse, if we delete the snapshot metadata first and then try to recreate things, omitting --disk-only fails because the verification code wants to force the default of an internal snapshot (which doesn't work with raw disks), and using --disk-only fails because the verification code doesn't see the 'disk-only' state in the snapshot xml - making it impossible to recreate the snapshot metadata. Ideally, the presence or absence of the --disk-only flag, and the presence or absence of an existing snapshot being overwritten, shouldn't matter; if the XML is valid for one situation, it should always be valid to redefine the metadata for that snapshot. Fix things by uniformly declaring that a redefined snapshot in the 'shutdown' state can permit a disk-only external snapshot (we got it right in only one out of three places in the function). See also https://bugzilla.redhat.com/1680304; this fixes the domain-agnostic problems mentioned there, but another patch is needed to fix further oddities with the qemu driver. I did not check for sure, but this has probably been broken even prior to when commit 670e86bf (1.1.4) factored redefine prep out of qemu code into the common snapshot_conf code. Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> --- src/conf/snapshot_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 489c25f511..3372c4df86 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1225,6 +1225,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; virDomainSnapshotObjPtr other; + bool offline = def->state == VIR_DOMAIN_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(def); /* Prevent circular chains */ if (def->parent) { @@ -1259,14 +1261,12 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } /* Check that any replacement is compatible */ - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && - def->state != VIR_DOMAIN_DISK_SNAPSHOT) { + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !offline) { virReportError(VIR_ERR_INVALID_ARG, _("disk-only flag for snapshot %s requires " "disk-snapshot state"), def->name); goto cleanup; - } if (def->dom && @@ -1315,8 +1315,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || - virDomainSnapshotDefIsExternal(def)) { + if (offline) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; } @@ -1346,7 +1345,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, *snap = other; } else { if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || + if (offline || def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; -- 2.20.1 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
