Re: [libvirt] [PATCHv2 03/13] snapshot: use metaroot node to simplify management
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)