[libvirt] [PATCH 4/8] snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE flag

[Peter Krempa]

On Fri, Jul 05, 2019 at 23:37:31 -0500, Eric Blake wrote:
> We've been doing a terrible job of performing XML validation in our
> various API that parse XML with a corresponding schema (we started
> with domains back in commit dd69a14f, v1.2.12, but didn't catch all
> domain-related APIs, and didn't cover other XMLM). New APIs (like

s/XMLM/XMLs/ ?

> checkpoints) should do the validation unconditionally, but it doesn't
> hurt to retrofit existing APIs to at least allow the option.  Wire up
> a new snapshot XML creation flag through all the hypervisors that
> support snapshots, as well as exposing it in 'virsh snapshot-create'.
> For 'virsh snapshot-create-as', we blindly set the flag without a
> command-line option, since the XML we create from the command line
> should always comply, but we have to add in code to disable validation
> if the server is too old to understand the flag.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  include/libvirt/libvirt-domain-snapshot.h |  2 ++
>  src/libvirt-domain-snapshot.c             |  3 +++
>  src/qemu/qemu_driver.c                    |  6 +++++-
>  src/test/test_driver.c                    |  6 +++++-
>  src/vbox/vbox_common.c                    | 11 ++++++++---
>  src/vz/vz_driver.c                        |  5 ++++-

The 'esx' driver also implements 'domainSnapshotCreateXML' as
esxDomainSnapshotCreateXML

>  tests/virsh-snapshot                      |  6 +++---
>  tools/virsh-snapshot.c                    | 15 ++++++++++++++-
>  tools/virsh.pod                           |  7 +++++--
>  9 files changed, 49 insertions(+), 12 deletions(-)

[...]

> diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
> index 0c8023d9f6..2687a34b96 100644
> --- a/src/libvirt-domain-snapshot.c
> +++ b/src/libvirt-domain-snapshot.c
> @@ -115,6 +115,9 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
>   * becomes current (see virDomainSnapshotCurrent()), and is a child
>   * of any previous current snapshot.
>   *
> + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, then @xmlDesc
> + * must validate against the <domainsnapshot> XML schema.

s/must validate/is validated/ ?

>   * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, then this
>   * is a request to reinstate snapshot metadata that was previously
>   * captured from virDomainSnapshotGetXMLDesc() before removing that

[...]

> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 54e31bec9d..8a912da50c 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -5487,6 +5487,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
>      nsresult rc;
>      resultCodeUnion result;
>      virDomainSnapshotPtr ret = NULL;
> +    unsigned int parse_flags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
> +                                VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE);

Parentheses unnecessary.

>      VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL;
> 
>      if (!data->vboxObj)

[...]

> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index e9f0ee0810..f7772ce549 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -50,6 +50,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer,
> 
>      snapshot = virDomainSnapshotCreateXML(dom, buffer, flags);
> 
> +    /* If no source file but validate was not recognized, try again without
> +     * that flag. */
> +    if (!snapshot && last_error->code == VIR_ERR_NO_SUPPORT && !from) {
> +        flags &= ~VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
> +        snapshot = virDomainSnapshotCreateXML(dom, buffer, flags);
> +    }

I think this compatibility glue is unnecessary ...

> +
>      /* Emulate --halt on older servers.  */
>      if (!snapshot && last_error->code == VIR_ERR_INVALID_ARG &&
>          (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {

[...]

> @@ -177,6 +188,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
>          flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
>      if (vshCommandOptBool(cmd, "live"))
>          flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> +    if (vshCommandOptBool(cmd, "validate"))
> +        flags |= VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
> 
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>          goto cleanup;
> @@ -366,7 +379,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
>      const char *desc = NULL;
>      const char *memspec = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> -    unsigned int flags = 0;
> +    unsigned int flags = VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;

... just to validate something we always generated ourselves.

ACK if you remove the use of the flag in cmdSnapshotCreateAs. Other are
at your discretion.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190708/8e68b76d/attachment-0001.sig>