[libvirt] [PATCH 2/2] qemu: Fix snapshot redefine vs. domain state bug
John Ferlan
jferlan at redhat.com
Tue Feb 26 00:01:06 UTC 2019
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 at redhat.com>
> ---
> 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 at redhat.com>
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
>
More information about the libvir-list
mailing list