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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190327/de3d4708/attachment-0001.sig>