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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190708/b983457a/attachment-0001.sig>