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: Eric Blake <eblake@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 602e5def597574a2c93dd13246d7d2a17ab044bb..90673ed0fb5082e7ae245555d0db6ace45fb0135 100644 (file)
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/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 */

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 47d95abd6d4e52a569ee1d8fec8b3490a3799feb..b98c72dc3f68cbd200561729ba245dbf59e430ce 100644 (file)
--- a/src/esx/esx_driver.c
+++ b/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;

diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 0c8023d9f68b2d4aea865d72ac3f116c96d40b8c..20a3bc5545b2e16863056015c1cc3c909c66d4dd 100644 (file)
--- 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
+ * 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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5a75f23981c6ea5fbdf737d5b460c4a44d9139ba..140896f329a53d36776797f8f53d8d28eaf58b4a 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/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;

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 49d7030d21fc7e7cb1397218e6216ad0402d4720..c10344f6cddc3f0c856cfa6f08cf5f9dc7c6006f 100644 (file)
--- a/src/test/test_driver.c
+++ b/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,

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 54e31bec9d0db741f0fa619b2b08af1d652e6b2a..8a912da50ce28f3010b56fa1996b8b2d1bbb0452 100644 (file)
--- 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);
     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;
 
 

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 2286f9a04f97b7a188eecb4db0b2988931e73828..50c883fecad619f496095759be541a729d2d3aa4 100644 (file)
--- a/src/vz/vz_driver.c
+++ b/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)))

diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
index cb498cf54e4b6f6d18a195e0aaffb79d0de7d6a8..8eab67c9e09e07ac1e0f771f4b183413bf75ff0b 100755 (executable)
--- a/tests/virsh-snapshot
+++ b/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
 

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index be11a7491cf86f7c4dc10ab941f848bfaa701743..b5e5c234cc380002d24a29fb5239e67879a1c346 100644 (file)
--- 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);
+    }
+
     /* 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"))

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 5168fa96b6911bd10656e1bc12b6fc27fb2c3371..dbcac24292783ff6d0d9a840d2137ecd4ecdf1d6 100644 (file)
--- a/tools/virsh.pod
+++ b/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