11:37 p.m.
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
checkpoints) should do the validation unconditionally, but it doesn't
hurt to retrofit existing APIs to at least allow the option. Wire 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 always comply, but we have to add in code to disable validation
if the server is too old to understand the flag.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt-domain-snapshot.h | 2 ++
src/libvirt-domain-snapshot.c | 3 +++
src/qemu/qemu_driver.c | 6 +++++-
src/test/test_driver.c | 6 +++++-
src/vbox/vbox_common.c | 11 ++++++++---
src/vz/vz_driver.c | 5 ++++-
tests/virsh-snapshot | 6 +++---
tools/virsh-snapshot.c | 15 ++++++++++++++-
tools/virsh.pod | 7 +++++--
9 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index 602e5def59..90673ed0fb 100644
--- 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/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 0c8023d9f6..2687a34b96 100644
--- 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
+ * must validate 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 9c05ab4ad1..97f3d7f786 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15508,7 +15508,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,
@@ -15549,6 +15550,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 7dd448bb20..e7ad4dbbd7 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7163,7 +7163,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;
@@ -7179,6 +7180,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 54e31bec9d..8a912da50c 100644
--- 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 2286f9a04f..50c883feca 100644
--- 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 cb498cf54e..8eab67c9e0 100755
--- 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 e9f0ee0810..f7772ce549 100644
--- 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;
@@ -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;
const vshCmdOpt *opt = NULL;
if (vshCommandOptBool(cmd, "no-metadata"))
diff --git a/tools/virsh.pod b/tools/virsh.pod
index dc39004a66..865fb2b0da 100644
--- 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 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
--
2.20.1