[libvirt] [PATCHv2 03/13] snapshot: use metaroot node to simplify management

[Eric Blake]

On 06/15/2012 04:23 AM, Peter Krempa wrote:
> On 06/15/12 06:18, Eric Blake wrote:
>> This idea was first suggested by Daniel Veillard here:
>> https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html
>>
>> Now that I am about to add more complexity to snapshot listing, it
>> makes sense to avoid code duplication and special casing for domain
>> listing (all snapshots) vs. snapshot listing (descendants); adding
>> a metaroot reduces the number of code lines by having the domain
>> listing turn into a descendant listing of the metaroot.
>>

>> +    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
> 
> I'm not quite sure what's the meaning of this statement. It efectively
> negates the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS flag, but the original code
> filtered it out.

^= toggles, not clears.  Due to (in hind-sight, a rather poor) choice on
my part in 0.9.7 when adding the ability to list children, we are stuck
with the following two bits with opposite meanings:

Bit 1 is named either VIR_DOMAIN_SNAPSHOT_LIST_ROOTS or
VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS.  When listing a domain, you use
_ROOTS, with the following effect:

virDomainSnapshotNum(,0) - list all snapshots (effectively, all
descendants of the metaroot)
virDomainSnapshotNum(,_ROOTS) - list all roots (effectively, the direct
children of the metaroot)

When listing a snapshot, you use _DESCENDANTS, with the following effect:

virDomainSnapshotNumChildren(,0) - list direct children
virDomainSnapshotNumChildren(,_DESCENDANTS) - list all descendants

So starting from a domain, we want to toggle the bit into something
where we can then start from the metaroot snapshot.  I'll add a comment.

> 
>> +    return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot,
>> names,
>> +                                                maxnames, flags);
> 
> This function calls virDomainSnapshotObjListCopyNames on each of
> children of the object but the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS is never
> checked. In fact virDomainSnapshotObjListCopyNames states that:
> 
>  "/* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, ..."

And this is precisely the case.  Since _ROOTS/_DESCENDANTS is bit one in
either case, in the caller we have:

_DESCENDANTS was specified (perhaps because _ROOTS was omitted):
  call ForEachDescendant
else 0 (direct children, perhaps because _ROOTS was supplied)
  call ForEachChild

> 
> I assume that it has to be filtered out:
> flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;

In other words, our choice of _which_ iterator to call determined
whether we have filtered the list according to _ROOTS/_DESCENDANTS, and
the callback doesn't have to care about the state of the bit.

>>   int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
>>                                   unsigned int flags)

>> +    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
>> +    return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);
> 
> Same comment as above.

Same answer as above.  :)

> ACK if the flag masking issue gets cleared. The metaroot approach is
> nice and consistent.

Then, assuming my explanation is okay, I will resolve this by adding
more comments to the code.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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