Re: [libvirt] [PATCH v4 06/20] backup: Document new XML for backups
[...]
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