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(a)redhat.com&gt; --- 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

The existing qemu snapshot code has a slight bug: if the domain is currently pmsuspended, you can't use the _REDEFINE flag even though the current domain state should have no bearing on being able to recreate metadata state; and conversely, you can use the _REDEFINE flag to create snapshot metadata claiming to be pmsuspended as a bypass to the normal restrictions that you can't create an original qemu snapshot in that state (the restriction against pmsuspend is specific to qemu, rather than part of the driver-agnostic snapshot_conf code). Fix this by factoring out the snapshot validation into a helper routine (which will also be useful in a later patch adding bulk redefine), and by adjusting the state being checked to the one supplied by the snapshot XML for redefinition, vs. the current domain state for original creation. However, since snapshots have one additional state beyond virDomainState (namely, the "disk-snapshot" state), and given the way virDomainSnapshotState was defined as an extension enum on top of virDomainState, we lose out on gcc's ability to force type-based validation that all switch branches are covered. In the process, observe that the qemu code already rejects _LIVE with _REDEFINE, but that this rejection occurs after parsing, and with a rather confusing message. Hoist that particular exclusive flag check earlier, so it gets a better error message, and is not inadvertently skipped when bulk redefine is added later (as that addition will occur prior to parsing). Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 117 +++++++++++++++++++++++++---------------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe1b7801e9..1f37107a20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15669,6 +15669,70 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver, } +/* Validate that a snapshot object does not violate any qemu-specific + * constraints. @state is virDomainState if flags implies creation, or + * virDomainSnapshotState if flags includes _REDEFINE (the latter + * enum is a superset of the former). */ +static int +qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state, + unsigned int flags) +{ + /* reject snapshot names containing slashes or starting with dot as + * snapshot definitions are saved in files named by the snapshot name */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid snapshot name '%s': " + "name can't contain '/'"), + def->name); + return -1; + } + + if (def->name[0] == '.') { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid snapshot name '%s': " + "name can't start with '.'"), + def->name); + return -1; + } + } + + /* allow snapshots only in certain states */ + switch (state) { + /* valid states */ + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_PAUSED: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_SHUTOFF: + case VIR_DOMAIN_CRASHED: + break; + + case VIR_DOMAIN_DISK_SNAPSHOT: + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + break; + + case VIR_DOMAIN_PMSUSPENDED: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("qemu doesn't support taking snapshots of " + "PMSUSPENDED guests")); + return -1; + + /* invalid states */ + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ + default: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), + virDomainSnapshotStateTypeToString(state)); + return -1; + } + + return 0; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -15703,6 +15767,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL); + VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, + NULL); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) @@ -15740,62 +15807,20 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup; - /* reject snapshot names containing slashes or starting with dot as - * snapshot definitions are saved in files named by the snapshot name */ - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { - if (strchr(def->name, '/')) { - virReportError(VIR_ERR_XML_DETAIL, - _("invalid snapshot name '%s': " - "name can't contain '/'"), - def->name); - goto cleanup; - } - - if (def->name[0] == '.') { - virReportError(VIR_ERR_XML_DETAIL, - _("invalid snapshot name '%s': " - "name can't start with '.'"), - def->name); - goto cleanup; - } - } + if (qemuDomainSnapshotValidate(def, redefine ? def->state : vm->state.state, + flags) < 0) + goto cleanup; /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) || - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - redefine)) { + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "with external checkpoints")); goto cleanup; } - /* allow snapshots only in certain states */ - switch ((virDomainState) vm->state.state) { - /* valid states */ - case VIR_DOMAIN_RUNNING: - case VIR_DOMAIN_PAUSED: - case VIR_DOMAIN_SHUTDOWN: - case VIR_DOMAIN_SHUTOFF: - case VIR_DOMAIN_CRASHED: - break; - - case VIR_DOMAIN_PMSUSPENDED: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("qemu doesn't support taking snapshots of " - "PMSUSPENDED guests")); - goto cleanup; - - /* invalid states */ - case VIR_DOMAIN_NOSTATE: - case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */ - case VIR_DOMAIN_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"), - virDomainStateTypeToString(vm->state.state)); - goto cleanup; - } - /* We are going to modify the domain below. Internal snapshots would use * a regular job, so we need to set the job mask to disallow query as * 'savevm' blocks the monitor. External snapshot will then modify the -- 2.20.1