From 95f8e3237e5486f487324c64e881b1fa714b7a5c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 5 Jul 2019 22:05:37 -0500 Subject: [PATCH] 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 Acked-by: Peter Krempa --- include/libvirt/libvirt-domain-snapshot.h | 2 ++ src/esx/esx_driver.c | 9 +++++++-- 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 +++++-- 10 files changed, 56 insertions(+), 14 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/esx/esx_driver.c b/src/esx/esx_driver.c index 47d95abd6d..b98c72dc3f 100644 --- 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 0c8023d9f6..20a3bc5545 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 + * is validated against the 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 5a75f23981..140896f329 100644 --- 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 49d7030d21..c10344f6cd 100644 --- 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 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 <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 5168fa96b6..dbcac24292 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4588,10 +4588,13 @@ used to represent properties of snapshots. =item B I [I] {[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 with the properties specified in -I. Normally, the only properties settable for a domain snapshot +I. 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 tool). Normally, +the only properties settable for a domain snapshot are the and elements, as well as if I<--disk-only> is given; the rest of the fields are ignored, and automatically filled in by libvirt. If I is -- GitLab