On 7/8/19 2:56 AM, Peter Krempa wrote:
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/ ?
Yes. Not sure how I let that one through, but I also spotted it locally before your mail.
The 'esx' driver also implements 'domainSnapshotCreateXML' as esxDomainSnapshotCreateXML
Fixed on my tree: diff --git i/src/esx/esx_driver.c w/src/esx/esx_driver.c index 47d95abd6d..9173896fe1 100644 --- i/src/esx/esx_driver.c +++ w/src/esx/esx_driver.c @@ -4101,18 +4101,23 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, bool diskOnly = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) != 0; bool quiesce = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) != 0; VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL; + unsigned int parse_flags = 0; /* ESX supports disk-only and quiesced snapshots; libvirt tracks no * snapshot metadata so supporting that flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | - VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + format_flags = VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; if (esxVI_EnsureSession(priv->primary) < 0) return NULL; def = virDomainSnapshotDefParseString(xmlDesc, priv->caps, - priv->xmlopt, NULL, 0); + priv->xmlopt, NULL, parse_flags); if (!def) return NULL;
+++ 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/ ?
Sure. Oddly enough, we do NOT document the VIR_DOMAIN_DEFINE_VALIDATE flag and friends; I guess I'll add another patch to my queue to rectify that. (Uggh, this pile of yak shavings at my desk is growing taller...)
+++ 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.
But compliant with the syntax-check, and allows for nicer indentation of the second line. Qemu just recently had a patch related to that: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01726.html
+++ 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 ...
It's necessary if snapshot-create-as uses the flag...
@@ -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.
...but I can drop the use here, if you think we are safe.
ACK if you remove the use of the flag in cmdSnapshotCreateAs. Other are at your discretion.
Okay, will drop the use in snapshot-create-as. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org