From dafef600f450ba1f335b3b95e6be3d04a01ca6a9 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 23 Feb 2019 13:21:38 -0600 Subject: [PATCH] snapshot: Permit redefine of offline external snapshot Due to historical back-compat, bare '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 with an error message that 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 still fails because the snapshot XML is not 'disk-only' - making it impossible to recreate the snapshot metadata (or to transfer it from one libvirtd host to another). 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 using virDomainSnapshotDefIsExternal() (caching the results up front, and eliminating other 'if' clauses now rendered redundant) when deciding whether the XML being requested for redefinition should permit external or force internal state capture (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 when the problems were introduced (git blame puts some affected hunks as far back as 1.0.0), but it was definitely been broken even before when commit 670e86bf (1.1.4) factored redefine prep out of qemu code into the common snapshot_conf code. Signed-off-by: Eric Blake Reviewed-by: John Ferlan --- src/conf/snapshot_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 3266c03c0e..41236d9932 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 external = 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) && !external) { 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 (external) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; } @@ -1346,8 +1345,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, *snap = other; } else { if (def->dom) { - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || - def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + if (external) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; }