Re: [PATCH 2/2] qemu: Fix snapshot redefine vs. domain state bug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux