[libvirt] [PATCH 0/5] snapshot XML parse cleanups (incremental backup saga)
During my checkpoint series, one of the review comments complained that setting the <creationTime> element during XML parsing to the current time is non-deterministic, which in turn makes writing testsuite additions difficult. The recommendation was to split out the non-deterministic defaults to a post-parse handler. Of course, since checkpoints copied from snapshots, I first need to fix snapshots to do the same, hence this series. Eric Blake (5): snapshot: Refactor snapshotxml2xml test snapshot: Don't expose testsuite-only state in snapshot XML snapshot: Factor out post-parse code snapshot: Allow for post-parse override snapshot: Use post-parse instead of regex in testsuite src/conf/domain_conf.h | 9 + src/conf/moment_conf.h | 2 + docs/schemas/domainsnapshot.rng | 1 - src/conf/domain_conf.c | 24 +++ src/conf/moment_conf.c | 19 ++ src/conf/snapshot_conf.c | 22 +-- src/libvirt_private.syms | 1 + .../disk_driver_name_null.xml | 1 + .../disk_snapshot.xml | 1 + tests/domainsnapshotxml2xmlout/empty.xml | 1 - .../domainsnapshotxml2xmlout/external_vm.xml | 1 + .../name_and_description.xml | 1 - tests/domainsnapshotxml2xmlout/noparent.xml | 2 +- tests/domainsnapshotxml2xmltest.c | 170 +++++++----------- 14 files changed, 137 insertions(+), 118 deletions(-) -- 2.20.1
Upcoming changes want to separate out a post-parse massaging of snapshots separate from parsing the XML, so as not to be dependent on filtering out an ever-changing timestamp from the testsuite. Along the way, this means we will want to add yet another conditional to the snapshot xml2xml tests on whether to perform post-processing steps to canned values. This will be easier to read if we consolidate all the decisions into a flags variable, instead of adding yet another boolean. While at it, drop the redundant inout test of "noparent" (once is enough). Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/domainsnapshotxml2xmltest.c | 63 +++++++++++++++++-------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 9f7f98585f..c329f15a54 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -33,6 +33,11 @@ static const char *testSnapshotXMLVariableLineRegexStr = regex_t *testSnapshotXMLVariableLineRegex = NULL; +enum { + TEST_INTERNAL = 1 << 0, + TEST_REDEFINE = 1 << 1, +}; + static char * testFilterXML(char *xml) { @@ -70,8 +75,7 @@ static int testCompareXMLToXMLFiles(const char *inxml, const char *outxml, const char *uuid, - bool internal, - bool redefine) + unsigned int flags) { char *inXmlData = NULL; char *outXmlData = NULL; @@ -82,12 +86,12 @@ testCompareXMLToXMLFiles(const char *inxml, unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; bool cur; - if (internal) { + if (flags & TEST_INTERNAL) { parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; formatflags |= VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL; } - if (redefine) + if (flags & TEST_REDEFINE) parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; if (virTestLoadFile(inxml, &inXmlData) < 0) @@ -108,7 +112,7 @@ testCompareXMLToXMLFiles(const char *inxml, formatflags))) goto cleanup; - if (!redefine) { + if (!(flags & TEST_REDEFINE)) { if (!(actual = testFilterXML(actual))) goto cleanup; @@ -135,8 +139,7 @@ struct testInfo { const char *inxml; const char *outxml; const char *uuid; - bool internal; - bool redefine; + unsigned int flags; }; @@ -146,7 +149,7 @@ testCompareXMLToXMLHelper(const void *data) const struct testInfo *info = data; return testCompareXMLToXMLFiles(info->inxml, info->outxml, info->uuid, - info->internal, info->redefine); + info->flags); } @@ -170,11 +173,11 @@ mymain(void) } -# define DO_TEST(prefix, name, inpath, outpath, uuid, internal, redefine) \ +# define DO_TEST(prefix, name, inpath, outpath, uuid, flags) \ do { \ const struct testInfo info = {abs_srcdir "/" inpath "/" name ".xml", \ abs_srcdir "/" outpath "/" name ".xml", \ - uuid, internal, redefine}; \ + uuid, flags}; \ if (virTestRun("SNAPSHOT XML-2-XML " prefix " " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ @@ -183,39 +186,43 @@ mymain(void) # define DO_TEST_IN(name, uuid) DO_TEST("in->in", name,\ "domainsnapshotxml2xmlin",\ "domainsnapshotxml2xmlin",\ - uuid, false, false) + uuid, 0) # define DO_TEST_OUT(name, uuid, internal) DO_TEST("out->out", name,\ "domainsnapshotxml2xmlout",\ "domainsnapshotxml2xmlout",\ - uuid, internal, true) + uuid, \ + internal | TEST_REDEFINE) -# define DO_TEST_INOUT(name, uuid, internal, redefine) \ +# define DO_TEST_INOUT(name, uuid, flags) \ DO_TEST("in->out", name,\ "domainsnapshotxml2xmlin",\ "domainsnapshotxml2xmlout",\ - uuid, internal, redefine) + uuid, flags) /* Unset or set all envvars here that are copied in qemudBuildCommandLine * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected * values for these envvars */ setenv("PATH", "/bin", 1); - DO_TEST_OUT("all_parameters", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", true); - DO_TEST_OUT("disk_snapshot_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", true); - DO_TEST_OUT("full_domain", "c7a5fdbd-edaf-9455-926a-d65c16db1809", true); - DO_TEST_OUT("noparent_nodescription_noactive", NULL, false); - DO_TEST_OUT("noparent_nodescription", NULL, true); - DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false); - DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", false); - DO_TEST_OUT("external_vm_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", false); + DO_TEST_OUT("all_parameters", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", + TEST_INTERNAL); + DO_TEST_OUT("disk_snapshot_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", + TEST_INTERNAL); + DO_TEST_OUT("full_domain", "c7a5fdbd-edaf-9455-926a-d65c16db1809", + TEST_INTERNAL); + DO_TEST_OUT("noparent_nodescription_noactive", NULL, 0); + DO_TEST_OUT("noparent_nodescription", NULL, TEST_INTERNAL); + DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 0); + DO_TEST_OUT("external_vm_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", + 0); - DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); - DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); - DO_TEST_INOUT("external_vm", NULL, false, false); - DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); - DO_TEST_INOUT("disk_snapshot", NULL, false, false); - DO_TEST_INOUT("disk_driver_name_null", NULL, false, false); + DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + DO_TEST_INOUT("external_vm", NULL, 0); + DO_TEST_INOUT("disk_snapshot", NULL, 0); + DO_TEST_INOUT("disk_driver_name_null", NULL, 0); DO_TEST_IN("name_and_description", NULL); DO_TEST_IN("description_only", NULL); -- 2.20.1
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 lackes 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 <eblake@redhat.com> --- 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 @@ <define name='state'> <choice> - <value>nostate</value> <value>running</value> <value>blocked</value> <value>paused</value> 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, "<description>%s</description>\n", def->common.description); - virBufferAsprintf(buf, "<state>%s</state>\n", - virDomainSnapshotStateTypeToString(def->state)); + if (def->state) + virBufferAsprintf(buf, "<state>%s</state>\n", + virDomainSnapshotStateTypeToString(def->state)); if (def->common.parent) { virBufferAddLit(buf, "<parent>\n"); @@ -821,8 +822,9 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</parent>\n"); } - virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n", - def->common.creationTime); + if (def->common.creationTime) + virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n", + def->common.creationTime); if (def->memory) { virBufferAsprintf(buf, "<memory snapshot='%s'", diff --git a/tests/domainsnapshotxml2xmlout/empty.xml b/tests/domainsnapshotxml2xmlout/empty.xml index 41538f7b74..0ae32e932a 100644 --- a/tests/domainsnapshotxml2xmlout/empty.xml +++ b/tests/domainsnapshotxml2xmlout/empty.xml @@ -1,6 +1,5 @@ <domainsnapshot> <name>1386166249</name> - <state>nostate</state> <creationTime>1386166249</creationTime> <domain> <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid> 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 @@ <domainsnapshot> <name>snap1</name> <description>A longer description!</description> - <state>nostate</state> </domainsnapshot> 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 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> - <state>nostate</state> + <state>running</state> <creationTime>1272917631</creationTime> <memory snapshot='internal'/> <domain> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index c329f15a54..a35edf18fb 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: * <name>1234352345</name> if the snapshot has no name, - * <creationTime>23523452345</creationTime>, - * <state>nostate</state> as the backend code doesn't fill this + * <creationTime>23523452345</creationTime> */ static const char *testSnapshotXMLVariableLineRegexStr = - "(<(name|creationTime)>[0-9]+</(name|creationTime)>|" - "<state>nostate</state>)"; + "<(name|creationTime)>[0-9]+</(name|creationTime)>"; regex_t *testSnapshotXMLVariableLineRegex = NULL; enum { TEST_INTERNAL = 1 << 0, TEST_REDEFINE = 1 << 1, + TEST_RUNNING = 1 << 2, }; static char * @@ -106,6 +105,11 @@ testCompareXMLToXMLFiles(const char *inxml, goto cleanup; if (cur) 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, @@ -219,7 +223,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); -- 2.20.1
Move the non-deterministic code that sets snapshot properties independently of what the incoming XML described to instead live in a post-parse function common to virDomainMoment (as checkpoints will also reuse it in later patches). This patch is just code motion, with no difference to any callers; but the next patch will further refactor things to call the post-process code from each driver, while the testsuite performs its own deterministic actions, for better testsuite coverage of parser/formatter code. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/moment_conf.h | 2 ++ src/conf/moment_conf.c | 19 +++++++++++++++++++ src/conf/snapshot_conf.c | 10 ++-------- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h index 8af3fbc6ac..e06a4a7b3c 100644 --- a/src/conf/moment_conf.h +++ b/src/conf/moment_conf.h @@ -39,4 +39,6 @@ struct _virDomainMomentDef { void virDomainMomentDefClear(virDomainMomentDefPtr def); +int virDomainMomentDefPostParse(virDomainMomentDefPtr def); + #endif /* LIBVIRT_MOMENT_CONF_H */ diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c index 886f3a44b4..0eb32eeb52 100644 --- a/src/conf/moment_conf.c +++ b/src/conf/moment_conf.c @@ -21,11 +21,14 @@ #include <config.h> +#include <sys/time.h> + #include "internal.h" #include "moment_conf.h" #include "domain_conf.h" #include "virlog.h" #include "viralloc.h" +#include "virstring.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -38,3 +41,19 @@ void virDomainMomentDefClear(virDomainMomentDefPtr def) VIR_FREE(def->parent); virDomainDefFree(def->dom); } + +/* Provide defaults for creation time and moment name after parsing XML */ +int +virDomainMomentDefPostParse(virDomainMomentDefPtr def) +{ + struct timeval tv; + + gettimeofday(&tv, NULL); + + if (!def->name && + virAsprintf(&def->name, "%lld", (long long)tv.tv_sec) < 0) + return -1; + + def->creationTime = tv.tv_sec; + return 0; +} diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 08f335646c..ef6eae3a51 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -23,7 +23,6 @@ #include <fcntl.h> #include <sys/stat.h> -#include <sys/time.h> #include <unistd.h> #include "internal.h" @@ -199,7 +198,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, size_t i; int n; char *creation = NULL, *state = NULL; - struct timeval tv; int active; char *tmp; char *memorySnapshot = NULL; @@ -210,8 +208,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, if (VIR_ALLOC(def) < 0) goto cleanup; - gettimeofday(&tv, NULL); - def->common.name = virXPathString("string(./name)", ctxt); if (def->common.name == NULL) { if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { @@ -219,8 +215,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, _("a redefined snapshot must have a name")); goto cleanup; } - if (virAsprintf(&def->common.name, "%lld", (long long)tv.tv_sec) < 0) - goto cleanup; } def->common.description = virXPathString("string(./description)", ctxt); @@ -276,8 +270,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } - } else { - def->common.creationTime = tv.tv_sec; + } else if (virDomainMomentDefPostParse(&def->common) < 0) { + goto cleanup; } memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); -- 2.20.1
Wire up the accessor functions necessary for the testsuite to install an alternative post-parse handler from normal drivers. I could have modified the signature for virDomainXMLOptionNew() to take another parameter, but thought it was easier to add a new set function rather than chase down all existing callers. Until code actually sets the override, there is no change in behavior. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 9 +++++++++ src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/snapshot_conf.c | 2 +- src/libvirt_private.syms | 1 + 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 988ef90f11..be3b8bedf2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2693,6 +2693,15 @@ virDomainXMLOptionPtr virDomainXMLOptionNew(virDomainDefParserConfigPtr config, virSaveCookieCallbacksPtr virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt); +typedef int (*virDomainMomentPostParseCallback)(virDomainMomentDefPtr def, + void *opaque); + +void virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentPostParseCallback cb, + void *opaque); +int virDomainXMLOptionRunMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentDefPtr def); + void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, virMacAddrPtr mac); virDomainXMLNamespacePtr diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17e8975be3..32d4539f69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -82,6 +82,10 @@ struct _virDomainXMLOption { /* Private data for save image stored in snapshot XML */ virSaveCookieCallbacks saveCookie; + + /* Snapshot postparse callbacks */ + virDomainMomentPostParseCallback moment; + void *moment_opaque; }; #define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ @@ -1451,6 +1455,26 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt) } +void +virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentPostParseCallback cb, + void *opaque) +{ + xmlopt->moment = cb; + xmlopt->moment_opaque = opaque; +} + + +int +virDomainXMLOptionRunMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentDefPtr def) +{ + if (!xmlopt->moment) + return virDomainMomentDefPostParse(def); + return xmlopt->moment(def, xmlopt->moment_opaque); +} + + void virBlkioDeviceArrayClear(virBlkioDevicePtr devices, int ndevices) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ef6eae3a51..36c328f692 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -270,7 +270,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } - } else if (virDomainMomentDefPostParse(&def->common) < 0) { + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->common) < 0) { goto cleanup; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b2be2d0fb9..8cd82c4416 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -604,6 +604,7 @@ virDomainWatchdogModelTypeToString; virDomainXMLOptionGetNamespace; virDomainXMLOptionGetSaveCookie; virDomainXMLOptionNew; +virDomainXMLOptionSetMomentPostParse; # conf/domain_event.h -- 2.20.1
On 4/16/19 9:53 AM, Eric Blake wrote:
Wire up the accessor functions necessary for the testsuite to install an alternative post-parse handler from normal drivers. I could have modified the signature for virDomainXMLOptionNew() to take another parameter, but thought it was easier to add a new set function rather than chase down all existing callers. Until code actually sets the override, there is no change in behavior.
I agree that extending DomainXMLOptionNew with yet another parameter would be a pain, I like this pattern better.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 9 +++++++++ src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ src/conf/snapshot_conf.c | 2 +- src/libvirt_private.syms | 1 + 4 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 988ef90f11..be3b8bedf2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2693,6 +2693,15 @@ virDomainXMLOptionPtr virDomainXMLOptionNew(virDomainDefParserConfigPtr config, virSaveCookieCallbacksPtr virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt);
+typedef int (*virDomainMomentPostParseCallback)(virDomainMomentDefPtr def, + void *opaque); + +void virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentPostParseCallback cb, + void *opaque); +int virDomainXMLOptionRunMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentDefPtr def); + void virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, virMacAddrPtr mac);
virDomainXMLNamespacePtr diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17e8975be3..32d4539f69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -82,6 +82,10 @@ struct _virDomainXMLOption {
/* Private data for save image stored in snapshot XML */ virSaveCookieCallbacks saveCookie; + + /* Snapshot postparse callbacks */ + virDomainMomentPostParseCallback moment; + void *moment_opaque; };
#define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ @@ -1451,6 +1455,26 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt) }
+void +virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentPostParseCallback cb, + void *opaque) +{ + xmlopt->moment = cb; + xmlopt->moment_opaque = opaque; +} +
Calling it 'moment' isn't very clear. The domain equivalent is domainPostParseCallback so I suggest momentPostParseCallback The moment_opaque pattern is weird to me, setting a value like that my first thought is it should be freed. And the stock MomentPostParse having a different signature than the callback confused me In the following patch the only usage is passing in 'timeptr' which is already global, can we drop moment_opaque and just access timeptr directly? If later usage actually needs to pass in opaque data, it should be modeled more like virDomainPostParse IMO Otherwise, for the series: Reviewed-by: Cole Robinson <crobinso@redhat.com> I don't mind if you adjust and push, or post a v2, or discuss further, etc Thanks, Cole
On 4/16/19 1:54 PM, Cole Robinson wrote:
On 4/16/19 9:53 AM, Eric Blake wrote:
Wire up the accessor functions necessary for the testsuite to install an alternative post-parse handler from normal drivers. I could have modified the signature for virDomainXMLOptionNew() to take another parameter, but thought it was easier to add a new set function rather than chase down all existing callers. Until code actually sets the override, there is no change in behavior.
I agree that extending DomainXMLOptionNew with yet another parameter would be a pain, I like this pattern better.
Yay, glad I picked it right, then :)
+ /* Snapshot postparse callbacks */ + virDomainMomentPostParseCallback moment; + void *moment_opaque; };
#define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ @@ -1451,6 +1455,26 @@ virDomainXMLOptionGetSaveCookie(virDomainXMLOptionPtr xmlopt) }
+void +virDomainXMLOptionSetMomentPostParse(virDomainXMLOptionPtr xmlopt, + virDomainMomentPostParseCallback cb, + void *opaque) +{ + xmlopt->moment = cb; + xmlopt->moment_opaque = opaque; +} +
Calling it 'moment' isn't very clear. The domain equivalent is domainPostParseCallback so I suggest momentPostParseCallback
Renaming is easy, I'll go with the longer name.
The moment_opaque pattern is weird to me, setting a value like that my first thought is it should be freed. And the stock MomentPostParse having a different signature than the callback confused me
In the following patch the only usage is passing in 'timeptr' which is already global, can we drop moment_opaque and just access timeptr directly? If later usage actually needs to pass in opaque data, it should be modeled more like virDomainPostParse IMO
Works for me. It feels a bit dirty depending on global state, but it's just a testsuite.
Otherwise, for the series:
Reviewed-by: Cole Robinson <crobinso@redhat.com>
I don't mind if you adjust and push, or post a v2, or discuss further, etc
I think I'll go ahead and just make the modifications you requested, then push; I'm making the same changes to my checkpoint code, where I hope to post v8 later today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Now that we can override the post-parse handling, let's update the testsuite to provide the desired timestamp/name rather than ignoring the non-deterministic one that was previously being generated. A few output files need timestamps added now that they are no longer ignored. Signed-off-by: Eric Blake <eblake@redhat.com> --- .../disk_driver_name_null.xml | 1 + .../disk_snapshot.xml | 1 + .../domainsnapshotxml2xmlout/external_vm.xml | 1 + tests/domainsnapshotxml2xmltest.c | 142 ++++++------------ 4 files changed, 52 insertions(+), 93 deletions(-) diff --git a/tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml b/tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml index ddd350de65..b0b27fd7e0 100644 --- a/tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml +++ b/tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml @@ -1,6 +1,7 @@ <domainsnapshot> <name>asdf</name> <description>adsf</description> + <creationTime>1555419243</creationTime> <disks> <disk name='vda' snapshot='external' type='file'> <source file='/tmp/foo'/> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 0e81f35c69..76c543d25c 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -1,6 +1,7 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> + <creationTime>1555419243</creationTime> <disks> <disk name='/dev/HostVG/QEMUGuest1'/> <disk name='hdb' snapshot='no'/> diff --git a/tests/domainsnapshotxml2xmlout/external_vm.xml b/tests/domainsnapshotxml2xmlout/external_vm.xml index 9da369b1e5..03c2e703b5 100644 --- a/tests/domainsnapshotxml2xmlout/external_vm.xml +++ b/tests/domainsnapshotxml2xmlout/external_vm.xml @@ -1,5 +1,6 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> + <creationTime>1555419243</creationTime> <memory snapshot='external' file='/dev/HostVG/GuestMemory'/> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index a35edf18fb..daceac34e6 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -21,55 +21,12 @@ static virQEMUDriver driver; -/* This regex will skip the following XML constructs in test files - * that are dynamically generated and thus problematic to test: - * <name>1234352345</name> if the snapshot has no name, - * <creationTime>23523452345</creationTime> - */ -static const char *testSnapshotXMLVariableLineRegexStr = - "<(name|creationTime)>[0-9]+</(name|creationTime)>"; - -regex_t *testSnapshotXMLVariableLineRegex = NULL; - enum { TEST_INTERNAL = 1 << 0, TEST_REDEFINE = 1 << 1, TEST_RUNNING = 1 << 2, }; -static char * -testFilterXML(char *xml) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - char **xmlLines = NULL; - char **xmlLine; - char *ret = NULL; - - if (!(xmlLines = virStringSplit(xml, "\n", 0))) { - VIR_FREE(xml); - goto cleanup; - } - VIR_FREE(xml); - - for (xmlLine = xmlLines; *xmlLine; xmlLine++) { - if (regexec(testSnapshotXMLVariableLineRegex, - *xmlLine, 0, NULL, 0) == 0) - continue; - - virBufferStrcat(&buf, *xmlLine, "\n", NULL); - } - - if (virBufferCheckError(&buf) < 0) - goto cleanup; - - ret = virBufferContentAndReset(&buf); - - cleanup: - virBufferFreeAndReset(&buf); - virStringListFree(xmlLines); - return ret; -} - static int testCompareXMLToXMLFiles(const char *inxml, const char *outxml, @@ -116,14 +73,6 @@ testCompareXMLToXMLFiles(const char *inxml, formatflags))) goto cleanup; - if (!(flags & TEST_REDEFINE)) { - if (!(actual = testFilterXML(actual))) - goto cleanup; - - if (!(outXmlData = testFilterXML(outXmlData))) - goto cleanup; - } - if (STRNEQ(outXmlData, actual)) { virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml); goto cleanup; @@ -143,15 +92,33 @@ struct testInfo { const char *inxml; const char *outxml; const char *uuid; + long long creationTime; unsigned int flags; }; +static long long timeptr; +static int +testSnapshotPostParse(virDomainMomentDefPtr def, void *opaque) +{ + long long *ptr = opaque; + + if (!*ptr) + return 0; + if (def->creationTime) + return -1; + def->creationTime = *ptr; + if (!def->name && + virAsprintf(&def->name, "%lld", def->creationTime) < 0) + return -1; + return 0; +} static int testCompareXMLToXMLHelper(const void *data) { const struct testInfo *info = data; + timeptr = info->creationTime; return testCompareXMLToXMLFiles(info->inxml, info->outxml, info->uuid, info->flags); } @@ -165,44 +132,34 @@ mymain(void) if (qemuTestDriverInit(&driver) < 0) return EXIT_FAILURE; - if (VIR_ALLOC(testSnapshotXMLVariableLineRegex) < 0) - goto cleanup; + virDomainXMLOptionSetMomentPostParse(driver.xmlopt, + testSnapshotPostParse, + &timeptr); - if (regcomp(testSnapshotXMLVariableLineRegex, - testSnapshotXMLVariableLineRegexStr, - REG_EXTENDED | REG_NOSUB) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "failed to compile test regex"); - goto cleanup; - } - - -# define DO_TEST(prefix, name, inpath, outpath, uuid, flags) \ +# define DO_TEST(prefix, name, inpath, outpath, uuid, time, flags) \ do { \ const struct testInfo info = {abs_srcdir "/" inpath "/" name ".xml", \ abs_srcdir "/" outpath "/" name ".xml", \ - uuid, flags}; \ + uuid, time, flags}; \ if (virTestRun("SNAPSHOT XML-2-XML " prefix " " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ } while (0) -# define DO_TEST_IN(name, uuid) DO_TEST("in->in", name,\ - "domainsnapshotxml2xmlin",\ - "domainsnapshotxml2xmlin",\ - uuid, 0) +# define DO_TEST_IN(name, uuid) DO_TEST("in->in", name, \ + "domainsnapshotxml2xmlin", \ + "domainsnapshotxml2xmlin", \ + uuid, 0, 0) -# define DO_TEST_OUT(name, uuid, internal) DO_TEST("out->out", name,\ - "domainsnapshotxml2xmlout",\ - "domainsnapshotxml2xmlout",\ - uuid, \ - internal | TEST_REDEFINE) +# define DO_TEST_OUT(name, uuid, time, internal) \ + DO_TEST("out->out", name, "domainsnapshotxml2xmlout", \ + "domainsnapshotxml2xmlout", uuid, time, internal | TEST_REDEFINE) -# define DO_TEST_INOUT(name, uuid, flags) \ - DO_TEST("in->out", name,\ +# define DO_TEST_INOUT(name, uuid, time, flags) \ + DO_TEST("in->out", name, \ "domainsnapshotxml2xmlin",\ "domainsnapshotxml2xmlout",\ - uuid, flags) + uuid, time, flags) /* Unset or set all envvars here that are copied in qemudBuildCommandLine * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected @@ -210,33 +167,32 @@ mymain(void) setenv("PATH", "/bin", 1); DO_TEST_OUT("all_parameters", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", - TEST_INTERNAL); + 1272917631, TEST_INTERNAL); DO_TEST_OUT("disk_snapshot_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", - TEST_INTERNAL); + 1272917631, TEST_INTERNAL); DO_TEST_OUT("full_domain", "c7a5fdbd-edaf-9455-926a-d65c16db1809", - TEST_INTERNAL); - DO_TEST_OUT("noparent_nodescription_noactive", NULL, 0); - DO_TEST_OUT("noparent_nodescription", NULL, TEST_INTERNAL); - DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); - DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 0); + 1272917631, TEST_INTERNAL); + DO_TEST_OUT("noparent_nodescription_noactive", NULL, 1272917631, 0); + DO_TEST_OUT("noparent_nodescription", NULL, 1272917631, TEST_INTERNAL); + DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", + 1272917631, 0); + DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", + 1272917631, 0); DO_TEST_OUT("external_vm_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", - 0); + 1272917631, 0); - DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", + 1386166249, 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); + 1272917631, TEST_RUNNING); + DO_TEST_INOUT("external_vm", NULL, 1555419243, 0); + DO_TEST_INOUT("disk_snapshot", NULL, 1555419243, 0); + DO_TEST_INOUT("disk_driver_name_null", NULL, 1555419243, 0); DO_TEST_IN("name_and_description", NULL); DO_TEST_IN("description_only", NULL); DO_TEST_IN("name_only", NULL); - cleanup: - if (testSnapshotXMLVariableLineRegex) - regfree(testSnapshotXMLVariableLineRegex); - VIR_FREE(testSnapshotXMLVariableLineRegex); qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.20.1
participants (2)
-
Cole Robinson -
Eric Blake