8:53 a.m.
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
8:53 a.m.
New subject: [libvirt] [PATCH 1/5] snapshot: Refactor snapshotxml2xml test
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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH 2/5] 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 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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH 3/5] snapshot: Factor out post-parse code
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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH 4/5] snapshot: Allow for post-parse override
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(a)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
1:54 p.m.
New subject: [libvirt] [PATCH 4/5] snapshot: Allow for post-parse override
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(a)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(a)redhat.com>
I don't mind if you adjust and push, or post a v2, or discuss further, etc
Thanks,
Cole
2:44 p.m.
New subject: [libvirt] [PATCH 4/5] snapshot: Allow for post-parse override
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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH 5/5] snapshot: Use post-parse instead of regex in testsuite
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(a)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
2050
days inactive
2050
days old
7 comments
2 participants
participants (2)
-
Cole Robinson -
Eric Blake