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