On 3/27/19 9:01 AM, Ján Tomko wrote:
On Wed, Mar 27, 2019 at 05:10:40AM -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.
+static virDomainCheckpointDefPtr +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + bool *current, + unsigned int flags) +{ + virDomainCheckpointDefPtr def = NULL; + virDomainCheckpointDefPtr ret = NULL; + xmlNodePtr *nodes = NULL; + size_t i; + int n; + char *creation = NULL; + struct timeval tv; + int active; + char *tmp; + + if (VIR_ALLOC(def) < 0) + goto cleanup; + + gettimeofday(&tv, NULL); +
Eww. I'd expect an XML parsing function to behave the same regardless of the time of day.
For domains, we'd put this kind of stuff into a post-parse function.
Ah, life has improved since I wrote snapshots. This is verbatim copy-and-paste from what snapshots currently do, but I agree that it would be nicer to first fix snapshots to switch to post-parse functions and then copy that fixed approach here (it would also make writing tests for this easier - I was struggling for how to write a meaningful test that does not just filter out all lines containing a dynamic timestamp - but if the test can supply its own post-parse hooks, we can provide a reproducible result).
+ + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { + if (virXPathLongLong("string(./creationTime)", ctxt, + &def->common.creationTime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing creationTime from existing checkpoint")); + goto cleanup; + } + + def->common.parent = virXPathString("string(./parent/name)", ctxt); +
+ if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
tmp is freed right away and the error reported on else is duplicate with the one a few lines below.
+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + + VIR_FREE(tmp); + if (!domainNode) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in checkpoint")); + goto cleanup; + } + def->common.dom = virDomainDefParseNode(ctxt->node->doc, domainNode, + caps, xmlopt, NULL, + domainflags); + if (!def->common.dom) + goto cleanup; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in checkpoint redefine")); + goto cleanup; + }
Yeah, there's probably a way to write that more succinctly. The snapshot code is hairier because it DOES permit an omitted <domain> subelement for back-compat reasons, but I didn't want to copy that here. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org