[libvirt] [PATCH v2 05/11] domain: Expand virDomainDefFormatInternal with snapshots

[Eric Blake]

On 2/27/19 10:27 AM, John Ferlan wrote:
> 
> 
> On 2/23/19 4:24 PM, Eric Blake wrote:
>> Make it possible to grab all snapshot XMLs via a single API
>> call, by adding a new internal flag.  All callers are adjusted,
>> for now passing NULL and not using the new flag. A new wrapper
>> virDomainDefFormatFull() is added to make it easier for the
> 
> Not really the next any more it seems - definitely in future ones though.
> 
>> next patch to add a few callers without having to revisit all
>> existing clients of virDomainDefFormat().
>>

>> @@ -28212,6 +28212,8 @@ virDomainVsockDefFormat(virBufferPtr buf,
>>  int
>>  virDomainDefFormatInternal(virDomainDefPtr def,
>>                             virCapsPtr caps,
>> +                           virDomainSnapshotObjListPtr snapshots,
>> +                           virDomainSnapshotObjPtr current_snapshot,
>>                             unsigned int flags,
>>                             virBufferPtr buf,
>>                             virDomainXMLOptionPtr xmlopt)
> 
> Rather than possibly continually adding parameters to
> virDomainDefFormatInternal, maybe we should take the plunge now to
> create a local struct that would be passed along that would fill in
> fields based on what "extra" format flags beyond the common set are
> being used.
> 

Ooh, nice idea, especially since I _will_ be adding more parameters for
bulk listing of checkpoints :)  Will do for v3 (since, as you point out,
everything beyond patch 1 is now 5.2 material).

> struct virDomainDefFormatInternalFlagsData {
>     virDomainSnapshotObjListPtr snapshots;
>     virDomainSnapshotObjPtr current_snapshot;
> };
> 
> I am of course assuming checkpoints in the future will do something similar.

Yep.


>> @@ -29016,6 +29025,23 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>
>>      virDomainSEVDefFormat(buf, def->sev);
>>
> 
> I think all of the next hunk should be in it's own helper method
> 
>> +    if (flags & VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS) {
>> +        unsigned int snapflags = flags & VIR_DOMAIN_DEF_FORMAT_SECURE ?
>> +            VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE : 0;
>> +
>> +        virBufferAddLit(buf, "<snapshots");
>> +        if (current_snapshot)
>> +            virBufferEscapeString(buf, " current='%s'",
>> +                                  current_snapshot->def->name);
>> +        virBufferAddLit(buf, ">\n");
>> +        virBufferAdjustIndent(buf, 2);
>> +        if (virDomainSnapshotObjListFormat(buf, uuidstr, snapshots, caps,
>> +                                           xmlopt, snapflags))

Can do.  Or maybe virDomainSnapshotObjListFormat() should do ALL of the
work (but then it needs a pointer to the current snapshot - but then
again, you also made me question whether the current snapshot should be
an internal detail to virDomainSnapshotObjList rather than tracked
separately, so that alone is a separate cleanup potentially worth doing
first).

> 
> So this answers my question from patch4...
> 
> Anyway, the return checking should be < 0 and I believe the @childrenBuf
> is how other similar constructs attempted for format the child XML
> output - I believe it could be used in this case as well or at least a
> similarly named/used childrenBuf in a method/helper.

I'm not sure a childrenBuf adds much. After all, whether we do:

most output direct to main buf
snapshot output to children buf
concatenate children buf to main buf

or

most output direct to main buf
snapshot output to main buf

we get the same result: on success, main buf is populated; on failure,
main buf may be half-built but it is going to be thrown away.


>> +char *
>> +virDomainDefFormatFull(virDomainDefPtr def, virCapsPtr caps,
>> +                       virDomainSnapshotObjListPtr snapshots,
>> +                       virDomainSnapshotObjPtr current_snapshot,
>> +                       unsigned int flags)
> 
> Naming is hard, but is "Full" just another way of saying "all flags" not
> just the common ones?
> 
> Not that we necessarily want to see N different implementations of
> virDomainDefFormatXXXX; however, if at some point in the future someone
> wanted to list all Checkpoints (;-)), but not all Snapshots - then would
> *Full still fit the bill or would we need a new method.

The alternative is to fix all current callers to pass the extra
parameter(s) even when they don't use them. Which may not be too bad, if
I take your advice to use a struct (so all existing callers pass NULL,
and don't have to be touched again), instead of my current approach of
adding yet more parameters for every additional thing to be printed.  I
don't necessarily need this helper function (test_driver.c was the only
immediate beneficiary), but having it reduced the churn (all other
non-qemu drivers have to be touched due to an added parameter, if I
don't add it).

> 
> Just want to be sure we rightly name it considering your knowledge of
> intended future changes.
> 
> It almost seems DefFormat is "format only common flags" while
> DefFormatFull would be format all flags that
> virDomainDefFormatConvertXMLFlags can handle beyond the original set.
> 
> If you feel *Full is fine - I'm not against it - just throwing out a
> something to consider...

It may go away once I implement your idea of an auxiliary struct (where
we have the one-time hit to other drivers to pass NULL, but then avoid
further churn).

> 
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS |
>> +                  VIR_DOMAIN_DEF_FORMAT_SNAPSHOTS, NULL);
>> +    if (virDomainDefFormatInternal(def, caps, snapshots, current_snapshot,
>> +                                   flags, &buf, NULL) < 0)
>> +        return NULL;
>> +
>> +    return virBufferContentAndReset(&buf);
>> +}
>> +
>> +
>>  char *
>>  virDomainDefFormat(virDomainDefPtr def, virCapsPtr caps, unsigned int flags)
>>  {
>> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> -
>>      virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL);
> 
> Considering virDomainDefFormatFull does check this - do we need to also
> check it?

This is slightly different than FormatFull() - that one accepts
VIR_DOMAIN_XML_SNAPSHOTS, while this one rejects it. (In other words, it
is one last safety check that no one requests snapshot XML without also
passing in a snapshots objlist to be printed).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org