Re: [libvirt] [PATCHv2 1/7] snapshot: new virDomainSnapshotListChildrenNames API

On 10/09/2011 09:13 PM, Daniel Veillard wrote: >On Fri, Sep 30, 2011 at 05:09:23PM -0600, Eric Blake wrote: >>The previous API addition allowed traversal up the hierarchy; >>this one makes it easier to traverse down the hierarchy. >> >>In the python bindings, virDomainSnapshotNumChildren can be >>generated, but virDomainSnapshotListChildrenNames had to copy >>from the hand-written example of virDomainSnapshotListNames. >> >>+/* Flags valid for virDomainSnapshotNum(), >>+ * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and >>+ * virDomainSnapshotListChildrenNames(). Note that the interpretation >>+ * of flag (1<<0) depends on which function it is passed to. */ >> typedef enum { >>- VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1<< 0), /* Filter by snapshots which >>- have no parents */ >>- VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1<< 1), /* Filter by snapshots which >>- have metadata */ >>+ VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1<< 0), /* Filter by snapshots >>+ with no parents, when >>+ listing a domain */ >>+ VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS = (1<< 0), /* List all descendants, >>+ not just children, when >>+ listing a snapshot */ >>+ VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1<< 1), /* Filter by snapshots >>+ which have metadata */ >> } virDomainSnapshotListFlags; > > Hum, okay, a tad bit confusing though Maybe so, but I though it was a tiny bit easier to overload a single bit than to waste a bit on both functions (that is, where 1<<0 can only be used on virDomainSnapshotNum, and 1<<2 can only be used on virDomainSnapshotNumChildren), when in reality, the point of the bit, for both functions, is to control whether recursion happens or whether the listing stops at the first round. Even more so if I add a meta-root element as part of the qemu refactoring, when the code for all roots of a domain vs. all direct children of a snapshot becomes identical :)

Yes, it looks a bit odd that '(flags & LIST_ROOTS) == 0' and '(flags & LIST_DESCENDANTS) != 0' are the conditions for recursion. But my rationale for the above design with the opposite bit sense of LIST_ROOTS vs. LIST_DESCENDANTS was that we want to default of each function to the cheaper computation, and at the time I wrote the patch, qemu listing direct children was O(n) but listing descendants was O(n^2), before I had written my later series to fix descendants to also be O(n). I had debated about naming the flag LIST_CHILDREN_ONLY, so that virDomainSnapshotNumChildren would report on descendants by default and require LIST_CHILDREN_ONLY to limit to direct children, so that the bit sense was identical (0 for recursion, non-zero for one level). If you think a matching bit sense is more important, then I can still make that change prior to 0.9.7. Only when we release are we locked in to the bit name and sense.

> ACK If we change the bit name, it will be as a separate patch between now and the 0.9.7 rc1. I'll push this as-is once I get through the rest of your series comments.