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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list