[libvirt] [PATCH v4 06/20] backup: Document new XML for backups

[John Ferlan]

[...]

I placed reviewing other changes higher over returning to this series to
answer, agree with, provide feedback, etc. on anything that you
responded to. Also, trying to avoid doing too many things at one time.

So I'll just provide some extra thoughts and wait for v5. I think we're
at a point where nickle and diming over minutiae is taking too much time
away from getting something upstream as soon as possible.

>>> +++ b/tests/domaincheckpointxml2xmlout/empty.xml
>>> @@ -0,0 +1,10 @@
>>> +<domaincheckpoint>
>>> +  <name>1525889631</name>
>>> +  <creationTime>1525889631</creationTime>
>>> +  <disks>
>>> +    <disk name='vda' checkpoint='bitmap' bitmap='1525889631'/>
>>> +  </disks>
>>> +  <domain>
>>> +    <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid>
>>
>> This looks strange without the name... and the domain type like the
>> sample.xml output has
> 
> Here's part of my reason for still having 'wip' on later patches - the
> snapshot code USED to just take a single UUID, until we released a
> couple of releases later that you can't revert to a domain state unless
> you capture the entire <domain> at the point in time you plan to revert
> to (as hot-plug actions after that time must be rolled back for the
> reversion to be correct).  So the snapshot code has a horrible hack of
> taking either a UUID or a full <domain> sub-element. I don't want to
> repeat that hack in <checkpoint> - I want a full <domain> element every
> time.  But coming up with the absolute minimum <domain> that does not
> make the test balloon into a super-long demonstration while still
> passing the RNG as a valid XML was where I got stuck, especially since
> the user does NOT pass in a <domain> sublement except when using the
> _REDEFINE flag (the rest of the time, the <domain> sublement is computed
> at creation time from the virDomainPtr that is gaining a new checkpoint).
> 

OK - makes sense with the additional details. Whatever you're
considering a minimum could just be added in.

> 
>>> +++ b/tests/domaincheckpointxml2xmlout/sample.xml
>>> @@ -0,0 +1,16 @@
>>> +<domaincheckpoint>
>>> +  <name>1525889631</name>
>>> +  <description>Completion of updates after OS install</description>
>>> +  <creationTime>1525889631</creationTime>
>>> +  <parent>
>>> +    <name>1525111885</name>
>>> +  </parent>
>>> +  <disks>
>>> +    <disk name='vda' checkpoint='bitmap' bitmap='1525889631'/>
>>
>> Maybe this one should show the size attribute too...
> 
> No, I want size to be an output-only element, and one you have to pass
> in an additional flag to get (because it requires runtime computation,
> rather than a static output).

Fair enough. It was mostly a well let's cover all the bases type note.

> 
> Oh, and that reminds me of another long-standing yak hair - we
> introduced the ability to validate some of our XML against the RNG, but
> we have not finished wiring it up to all of the APIs that take XML.
> Domain snapshots don't have a validation flag, and hence my
> copy-and-paste code for checkpoints don't have one, but both need one.
> (Otherwise, the presence or absence of an ignored <size> element won't
> really matter whether the RNG permits or rejects it)
> 

So avoiding domain snapshot code isn't only something I've done then!

John