On Tue, Jun 18, 2019 at 22:47:45 -0500, Eric Blake wrote:
Add a new file checkpoint_conf.c that performs the translation to and from new XML describing a checkpoint. The code shares a common base class with snapshots, since a checkpoint similarly represents the domain state at a moment in time. Add some basic testing of round trip XML handling through the new code.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/checkpoint_conf.h | 94 +++ src/conf/virconftypes.h | 3 + po/POTFILES | 1 + src/conf/Makefile.inc.am | 2 + src/conf/checkpoint_conf.c | 575 ++++++++++++++++++ src/libvirt_private.syms | 8 + tests/Makefile.am | 9 +- .../internal-active-invalid.xml | 53 ++ .../internal-inactive-invalid.xml | 53 ++ tests/domaincheckpointxml2xmltest.c | 223 +++++++ 10 files changed, 1019 insertions(+), 2 deletions(-) create mode 100644 src/conf/checkpoint_conf.h create mode 100644 src/conf/checkpoint_conf.c create mode 100644 tests/domaincheckpointxml2xmlout/internal-active-invalid.xml create mode 100644 tests/domaincheckpointxml2xmlout/internal-inactive-invalid.xml create mode 100644 tests/domaincheckpointxml2xmltest.c
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c new file mode 100644 index 0000000000..a7a34d3887 --- /dev/null +++ b/src/conf/checkpoint_conf.c @@ -0,0 +1,575 @@
[...]
+static int +virDomainCheckpointDiskDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainCheckpointDiskDefPtr def) +{ + int ret = -1; + char *checkpoint = NULL; + char *bitmap = NULL; + xmlNodePtr saved = ctxt->node; + + ctxt->node = node; + + def->name = virXMLPropString(node, "name"); + if (!def->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name from disk checkpoint element"));
Shouldn't schema validation catch this? If schema validation is not enabled note that there isn't a second chance to do it.
+ goto cleanup; + }
[...]
+ + if (!(def = virDomainCheckpointDefNew())) + return NULL; + + def->parent.name = virXPathString("string(./name)", ctxt); + if (def->parent.name == NULL) { + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("a redefined checkpoint must have a name")); + goto cleanup; + } + } + + def->parent.description = virXPathString("string(./description)", ctxt); + + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { + if (virXPathLongLong("string(./creationTime)", ctxt, + &def->parent.creationTime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing creationTime from existing checkpoint"));
While I can see the point that we do the same thing in the snapshot XML parser, having validation intermixed with the parser is not great for the future.
+ goto cleanup; + } + + def->parent.parent_name = virXPathString("string(./parent/name)", ctxt); + + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
ACK, this adds bunch of technical debt based on the fact that it's mostly copied form snapshots, but I trust you will address that later. I'd strongly prefer if we'd do schema validation unconditionally for the new APIs though since we won't get to fix that one later.