snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE flag

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, didn't document the use of the flag, and didn't
cover other XML). New APIs (like checkpoints) should do the validation
unconditionally, but it doesn't hurt to continue retrofitting existing
APIs to at least allow the option.

While there are many APIs that could be improved, this patch focuses
on wiring 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 generally always comply (note that validation
might cause failures where it used to succeed, such as if we tighten
the RNG to reject a name of '../\n'); but blindly passing the flag
means we also have to add in fallback code to disable validation if
the server is too old to understand the flag.
Signed-off-by: NEric Blake <eblake@redhat.com>
Acked-by: NPeter Krempa <pkrempa@redhat.com>

include/libvirt/libvirt-domain-snapshot.h

@@ -71,6 +71,8 @@ typedef enum {
     VIR_DOMAIN_SNAPSHOT_CREATE_LIVE        = (1 << 8), /* create the snapshot
                                                           while the guest is
                                                           running */
+    VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE    = (1 << 9), /* validate the XML
+                                                          against the schema */
 } virDomainSnapshotCreateFlags;
 
 /* Take a snapshot of the current VM state */

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)
+        parse_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;

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
+ * is validated against the <domainsnapshot> XML schema.
+ *
  * 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

src/qemu/qemu_driver.c

@@ -15525,7 +15525,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
                   VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
                   VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
                   VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
-                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
+                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
+                  VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL);
 
     VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
                          VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
@@ -15566,6 +15567,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
         !virDomainObjIsActive(vm))
         parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE;
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
+        parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
+
     if (!(def = virDomainSnapshotDefParseString(xmlDesc, caps, driver->xmlopt,
                                                 NULL, parse_flags)))
         goto cleanup;

src/test/test_driver.c

@@ -7183,7 +7183,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
         VIR_DOMAIN_SNAPSHOT_CREATE_HALT |
         VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
         VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
-        VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
+        VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
+        VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL);
 
     if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)))
         update_current = false;
@@ -7199,6 +7200,9 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
         goto cleanup;
     }
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
+        parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
+
     if (!(def = virDomainSnapshotDefParseString(xmlDesc,
                                                 privconn->caps,
                                                 privconn->xmlopt,

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);
     VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL;
 
     if (!data->vboxObj)
@@ -5496,12 +5498,15 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
     /* VBox has no snapshot metadata, so this flag is trivial.  */
     virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA |
                   VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
-                  VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, NULL);
+                  VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT |
+                  VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL);
+
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
+        parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
 
     if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps,
                                                 data->xmlopt, NULL,
-                                                VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
-                                                VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE)))
+                                                parse_flags)))
         goto cleanup;
 
 

src/vz/vz_driver.c

@@ -2586,7 +2586,7 @@ vzDomainSnapshotCreateXML(virDomainPtr domain,
     bool job = false;
     VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL;
 
-    virCheckFlags(0, NULL);
+    virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL);
 
     if (!(dom = vzDomObjFromDomain(domain)))
         return NULL;
@@ -2594,6 +2594,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain,
     if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, dom->def, flags) < 0)
         goto cleanup;
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
+        parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
+
     if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps,
                                                 driver->xmlopt, NULL,
                                                 parse_flags)))

tests/virsh-snapshot

@@ -180,11 +180,11 @@ compare exp err || fail=1
 # Restore state with redefine
 $abs_top_builddir/tools/virsh -c test:///default >out 2>err <<EOF || fail=1
   # Redefine must be in topological order; this will fail
-  snapshot-create test --redefine s2.xml
+  snapshot-create test --redefine s2.xml --validate
   echo --err marker
   # This is the right order
-  snapshot-create test --redefine s3.xml
-  snapshot-create test --redefine s2.xml --current
+  snapshot-create test --redefine s3.xml --validate
+  snapshot-create test --redefine s2.xml --current --validate
   snapshot-info test --current
 EOF
 

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);
+    }
+
     /* Emulate --halt on older servers.  */
     if (!snapshot && last_error->code == VIR_ERR_INVALID_ARG &&
         (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
@@ -147,6 +154,10 @@ static const vshCmdOptDef opts_snapshot_create[] = {
      .help = N_("require atomic operation")
     },
     VIRSH_COMMON_OPT_LIVE(N_("take a live snapshot")),
+    {.name = "validate",
+     .type = VSH_OT_BOOL,
+     .help = N_("validate the XML against the schema"),
+    },
     {.name = NULL}
 };
 
@@ -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;
@@ -383,7 +396,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;
     const vshCmdOpt *opt = NULL;
 
     if (vshCommandOptBool(cmd, "no-metadata"))

tools/virsh.pod

@@ -4588,10 +4588,13 @@ used to represent properties of snapshots.
 
 =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]]
 | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>]
-[I<--quiesce>] [I<--atomic>] [I<--live>]}
+[I<--quiesce>] [I<--atomic>] [I<--live>]} [I<--validate>]
 
 Create a snapshot for domain I<domain> with the properties specified in
-I<xmlfile>.  Normally, the only properties settable for a domain snapshot
+I<xmlfile>.   Optionally, the I<--validate> option can be passed to
+validate the format of the input XML file against an internal RNG
+schema (identical to using the L<virt-xml-validate(1)> tool). Normally,
+the only properties settable for a domain snapshot
 are the <name> and <description> elements, as well as <disks> if
 I<--disk-only> is given; the rest of the fields are
 ignored, and automatically filled in by libvirt.  If I<xmlfile> is