On 2/23/19 3:00 PM, Eric Blake wrote: > 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. Would it be worth adding some sort of compiler warning that VIR_DOMAIN_SNAPSHOT_STATE_LAST must be VIR_DOMAIN_DISK_SNAPSHOT + 1. That would cause future additions to virDomainSnapshotState to rework the math - verify seems to be used for some places. Perhaps something worthwhile for any other one of these overlaid structs. > > 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(-) > It seems there are multiple things going on, some code refactoring and then a couple bug fixes. IDC if they're combined - separating for easier backporting is always nice... > 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)); I assume this is part of the fix - is there perhaps a way or a desire to distinguish between this and the invalid states below? That is a slightly different message to more easily know what went wrong without having to dig into the code. > + 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); And perhaps this is a 3rd bug fix in the same patch . > > 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, Is passing of a different state value the 2nd bug? > + 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)) { Was this part of what I called the 3rd bug? Probably would be good to separate if you think that's wise/feasible. Since this is a bug fix it is a change that could be accepted post freeze. I'll let you decide how to proceed - Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> John > + 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
