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

On 2/6/19 2:18 PM, Eric Blake wrote: s/../. I think most of the description beyond the first sentence or so is covered in the docs and probably doesn't need to be in the commit. Perhaps just " Prepare for new checkpoint and backup APIs by describing the XML that will represent a checkpoint and backup. The checkpoint XML is modeled heavily after virDomainSnapshotPtr, while the backup XML is provides newer capabilities for both push and pull models. Hmmm... Should this paragraph be in the commit message or under the ---? BTW: Only after reading through all this does this partially make sense. Lots of detail and files... I know they're related, but if there's a way to split it may be nicer for review processing ;-) QEMU s/concert/conjunction (although I understood concert, I think conjunction is more apropos) s/,// The optional name I would think the description of what happens when a name is specified would be obvious. An optional human-... s/domain (/domain. In/ QEMU s/); if/. If/ each disk participating in the checkpoint ? Or perhaps "On output, this is the state of the domain's list of disks relative to how the checkpoint is used for each." [ - or something like that] This is tough to read as one fairly long paragraph especially when it comes to picking out the attributes, consider: <dt><code>disk</code></dt> <dd>This sub-element describes the checkpoint properties of a specific disk with the following attributes: <dl> <dt><code>name</code></dt> <dd>The attribute <code>name</code> is mandatory and must match either the <code>&lt;target dev='name'/&gt;</code> or an unambiguous <code>&lt;source file='name'/&gt;</code> of one of the <a href="formatdomain.html#elementsDisks">disk devices</a> specified for the domain at the time of the checkpoint.</dd> <dt><code>checkpoint</code></dt> <dd>The attribute <code>checkpoint</code> is optional on input. Possible values are <code>no</code> when the disk does not participate in this checkpoint or <code>bitmap</code> if the disk will track all changes since the creation of this checkpoint via a bitmap.</dd> <dt><code>bitmap</code></dt> <dd>Optional attribute to define the name of the tracking bitmap (defaulting to the checkpoint name) when the disk is participating in the checkpoint processing.</dd> <dt><code>size</code></dt> <dd>On output, this attribute may be present if the <code>VIR_DOMAIN_CHECKPOINT_XML_SIZE</code> flag was used to perform a dynamic query of the estimated size in bytes of the changes made since the checkpoint was created.</dd> </dl> A readonly representation of the time this checkpoint was created. An optional readonly representation of the parent of this checkpoint. s/name/<code>name</code> A readonly representation of the inactive ... And then just remove the Readonly single word. s/,/. The/ s/model,/model/ An optional element to describe the name of an existing checkpoint... [one would hope we wouldn't have to give a link to the above named checkpoints, but I suppose it would be possible - your call on that] s/one (in the/one. In the/ s/;/./ s/in/In/ s/)./. [could provide a link to the domain disk section if you feel it's appropriate or necessary] s/started/is started/ An optional listing s/, and/ and/ s/ job././ [job is an implementation detail] This one's harder to read than the other one...Consider: <dt><code>disk</code></dt> <dd>This sub-element describes the backup properties of a specific disk with the following attributes: <dl> <dt><code>name</code></dt> <dd>A mandatory attribute and must match either the <code>&lt;target dev='name'/&gt;</code> or an unambiguous <code>&lt;source file='name'/&gt;</code> of one of the <a href="formatdomain.html#elementsDisks">disk devices</a> specified for the domain at the time of the checkpoint.</dd> <dt><code>type</code></dt> <dd>An optional attribute to describe the type of disk. Valid values can be <code>file</code>, <code>block</code>, or <code>network</code>. Similar to a disk declaration for a domain, controls what additional sub-elements are needed to describe the destination (such as <code>protocol</code> for a network destination).</dd> <dt><code>target</code></dt> <dd>For push mode backups, this is the primary sub-element to describe the file name to be used during the backup operation similar to the <code>source</code> sub-element of a domain disk. An optional sub-element <code>driver</code> can also be used, with an attribute <code>type</code> to specify a destination format different from qcow2. Additionally, if the push backup is not incremental, <code>target</code> may contain an optional attribute <code>shallow="on"</code> so that the destination file copies only the top-most source file in a backing chain, rather than collapsing the entire chain into the copy.</dd> <dt><code>scratch</code></dt> <dd>For pull mode backups, this is the primary sub-element to describe the file name to be used during the backup operation similar to the <code>source</code> sub-element of a domain disk.</dd> </dl> </dd> Perhaps an example of mode="push" would be useful as that <target> sub-element has a lot of "nuances" (kindly said ;-)) The 'id' is not described in docs nor do I see it in any of your output. Remnant or future? More work? leaving a golden egg like this keeps me interested ;-) Could something be done to create some "common" include file that would allow for sharing w/ diskSourceNetworkHost so that when/if updating domaincommon someone doesn't forget or neglect to update the domainbackup? One difference I see would be rdma transport which would need to be handled specially in domainbackup XML processing since I assume it wouldn't be supported. Is there still work to be done here! So the docs describe attribute type having "file", "block", or "network" and this is different. Things should match I would think. Missing optional attribute "shallow" w/ OnOff as describe in docs. Same w/r/t to valid 'type' values. I haven't looked ahead to conf processing, but I assume this is the same type as whatever this gets read into. Whenever I see size in reference to disks I'm thinking ULL's. Should add a 'block' and 'network' example too for completeness - of course that depends on the matching between docs and rng. Should add target w/ shallow=on for completeness. Yes, I recall the commit message indicating essentially minimal. This looks strange without the name... and the domain type like the sample.xml output has Maybe this one should show the size attribute too... I need to stop here for the day, but will continue... John