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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

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:

# 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]