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

[Daniel Veillard]

On Mon, Oct 10, 2011 at 05:07:47PM -0600, Eric Blake wrote:
> 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

> 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.

  well I think that's fine, and ow the explanations are archived so
  someone confused can get the rationale easily :-)

[...]
> >   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.

  just push !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/