From a007fcab3b9b760db5fc1ffee7ed75398a5d3125 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 16 Apr 2019 17:44:38 -0500 Subject: [PATCH] snapshot: Don't expose testsuite-only state in snapshot XML None of the existing drivers actually use the 0-valued 'nostate' snapshot state; rather, it was a fluke of implementation. In fact, some drivers, like qemu, actively reject 'nostate' as invalid during a snapshot redefine. Normally, a driver computes the state post-parse from the current domain, and thus virDomainSnapshotGetXMLDesc() will never expose the state. However, since the testsuite lacks any associated domain to copy state from, and lacks post-parse processing that normal drivers have, the testsuite output had several spots with the state, coupled with a regex filter to ignore the oddity. It is better to follow the lead of other XML defaults, by not outputting anything during format if post-parse defaults have not been applied, and rejecting the default value during parsing. The testsuite needs a bit of an update, by adding another flag for when to simulate a post-parse action of setting a snapshot state, but none of the drivers are impacted other than rejecting XML that was previously already suspicious in nature. Similarly, don't expose creation time 0 (for now, only possible if a user redefined a snapshot to claim creation at the Epoch, but also happens once setting the creation time is deferred to a post-parse handler). This is also a step towards cleaning up snapshot_conf.c to separate its existing post-parse work (namely, setting the creationTime and default snapshot name) from the pure parsing work, so that we can get rid of the testsuite hack of regex filtering of the XML and instead have more accurate testing of our parser/formatter code. Signed-off-by: Eric Blake Reviewed-by: Cole Robinson --- docs/schemas/domainsnapshot.rng | 1 - src/conf/snapshot_conf.c | 12 +++++++----- tests/domainsnapshotxml2xmlout/empty.xml | 1 - .../name_and_description.xml | 1 - tests/domainsnapshotxml2xmlout/noparent.xml | 2 +- tests/domainsnapshotxml2xmltest.c | 15 ++++++++++----- 6 files changed, 18 insertions(+), 14 deletions(-) diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 2680887095..8863d99578 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -103,7 +103,6 @@ - nostate running blocked paused diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ce543cbaf7..08f335646c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -245,7 +245,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, goto cleanup; } def->state = virDomainSnapshotStateTypeFromString(state); - if (def->state < 0) { + if (def->state <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid state '%s' in domain snapshot XML"), state); @@ -810,8 +810,9 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, if (def->common.description) virBufferEscapeString(buf, "%s\n", def->common.description); - virBufferAsprintf(buf, "%s\n", - virDomainSnapshotStateTypeToString(def->state)); + if (def->state) + virBufferAsprintf(buf, "%s\n", + virDomainSnapshotStateTypeToString(def->state)); if (def->common.parent) { virBufferAddLit(buf, "\n"); @@ -821,8 +822,9 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "\n"); } - virBufferAsprintf(buf, "%lld\n", - def->common.creationTime); + if (def->common.creationTime) + virBufferAsprintf(buf, "%lld\n", + def->common.creationTime); if (def->memory) { virBufferAsprintf(buf, " 1386166249 - nostate 1386166249 9d37b878-a7cc-9f9a-b78f-49b3abad25a8 diff --git a/tests/domainsnapshotxml2xmlout/name_and_description.xml b/tests/domainsnapshotxml2xmlout/name_and_description.xml index 435ab79aa9..90ce774741 100644 --- a/tests/domainsnapshotxml2xmlout/name_and_description.xml +++ b/tests/domainsnapshotxml2xmlout/name_and_description.xml @@ -1,5 +1,4 @@ snap1 A longer description! - nostate diff --git a/tests/domainsnapshotxml2xmlout/noparent.xml b/tests/domainsnapshotxml2xmlout/noparent.xml index d4360f0dc1..0cbbb658d8 100644 --- a/tests/domainsnapshotxml2xmlout/noparent.xml +++ b/tests/domainsnapshotxml2xmlout/noparent.xml @@ -1,7 +1,7 @@ my snap name !@#$%^ - nostate + running 1272917631 diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 056219e849..191612d216 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -24,18 +24,17 @@ static virQEMUDriver driver; /* This regex will skip the following XML constructs in test files * that are dynamically generated and thus problematic to test: * 1234352345 if the snapshot has no name, - * 23523452345, - * nostate as the backend code doesn't fill this + * 23523452345 */ static const char *testSnapshotXMLVariableLineRegexStr = - "(<(name|creationTime)>[0-9]+|" - "nostate)"; + "<(name|creationTime)>[0-9]+"; regex_t *testSnapshotXMLVariableLineRegex = NULL; enum { TEST_INTERNAL = 1 << 0, /* Test use of INTERNAL parse/format flag */ TEST_REDEFINE = 1 << 1, /* Test use of REDEFINE parse flag */ + TEST_RUNNING = 1 << 2, /* Set snapshot state to running after parse */ }; static char * @@ -109,6 +108,11 @@ testCompareXMLToXMLFiles(const char *inxml, goto cleanup; formatflags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT; } + if (flags & TEST_RUNNING) { + if (def->state) + goto cleanup; + def->state = VIR_DOMAIN_RUNNING; + } if (!(actual = virDomainSnapshotDefFormat(uuid, def, driver.caps, driver.xmlopt, @@ -222,7 +226,8 @@ mymain(void) 0); DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); - DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", + TEST_RUNNING); DO_TEST_INOUT("external_vm", NULL, 0); DO_TEST_INOUT("disk_snapshot", NULL, 0); DO_TEST_INOUT("disk_driver_name_null", NULL, 0);