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