Re: [Qemu-devel] [RFC V3 3/7] qapi: Add skeletton of command to query a drive bs graph.

["Benoît Canet" <benoit.canet@irqsave.net>]

Le Wednesday 04 Dec 2013 à 16:46:34 (-0700), Eric Blake a écrit :
> On 12/03/2013 06:26 AM, Benoît Canet wrote:
> 
> In addition to Fam's review,
> 
> s/skeletton/skeleton/ in subject
> 
> > ---
> >  blockdev.c       |  8 ++++++++
> >  qapi-schema.json | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index a474bb5..824e718 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1940,6 +1940,14 @@ void qmp_drive_backup(const char *device, const char *target,
> >      }
> >  }
> >  
> > +BlockGraphNode * qmp_query_drive_graph(const char *device, Error **errp)
> 
> Style: no space after *
> 
> > +{
> > +    /* the implementation of this function would recurse through the
> > +     * BlockDriverState graph to build it's result
> > +     */
> > +    return NULL;
> 
> Shouldn't you set errp when returning failure?
> 
> > +++ b/qapi-schema.json
> > @@ -2008,6 +2008,38 @@
> >  { 'command': 'drive-backup', 'data': 'DriveBackup' }
> >  
> >  ##
> > +# @BlockGraphNode
> > +#
> > +# Information about a node of the block driver state graph
> > +#
> > +# @node-name: the name of the node in the graph
> > +#
> > +# @drv: the name of the block format used by this node as described in
> > +#       @BlockDeviceInfo.
> 
> It would be nice if BlockDeviceInfo and BlockGraphNode used an enum
> rather than an open-coded string for this field.
> 
> > +#
> > +# @children: a list of @BlockGraphNode being the children of this node
> 
> s/being/that are/
> 
> > +##
> > +# @query-drive-graph
> > +#
> > +# Get the block driver states graph for a given drive
> > +#
> > +# @device: the name of the device to get the graph from
> > +#
> > +# Returns: the root @BlockGraphNode
> > +#
> > +# Since 1.8
> > +##
> > +{ 'command': 'query-drive-graph',
> > +  'data': { 'device': 'str' },
> > +  'returns': 'BlockGraphNode' }
> 
> Am I correct that it will be possible to have named nodes that are not
> currently associated with any device?  If so, how do we learn about
> those nodes?  Would it be better to have a command that returns an array
> of structs for all known node roots, with an optional member describing
> which device owns that node root?  Something like:

The code have a list of all named nodes but not a list of named nodes roots.
Also it's difficult to get the device name for a named node because the bses don't
have any backward pointers to their parents.
It could be done by recursing into all the blockbackend bs but it's twisted.

In fact I am wondering if we really need something to spit out the named nodes
topology in QMP for the simple reason that the names of the nodes are given by the
management so the management should already know the topology.

Best regards

Benoît

> 
> # Represent a root of a block graph
> # @root: a named node forming a root of a node graph
> # @device: #optional device name that owns this root
> { 'type': 'BlockGraphRoot',
>   'data': { 'root': 'BlockGraphNode',
>             '*device': 'str' } }
> 
> # @query_drive-graphs
> # Returns an array of all node graph roots
> { 'command': 'query-drive-graphs',
>   'returns': [ 'BlockGraphRoot' ] }
> 
> possibly with 'data':{'*device':'str'} to allow filtering to just a
> 1-element array based on the device name (although I'm not sure if
> providing the complexity of filtering is worth it).
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>