Re: [libvirt] [PATCH v7 09/23] backup: Parse and output checkpoint XML

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.

> + > +    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; > +        }