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