Re: [Qemu-devel] [PATCH 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Eric Blake <eblake@redhat.com>]

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

On 10/28/2014 11:04 PM, Fam Zheng wrote:
> This bool option will allow query all the node names. It iterates all
> the BDSes that are assigned a name, also in this case don't query up the
> backing chain.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/qapi.c         | 20 +++++++++++++-------
>  hmp.c                |  2 +-
>  qapi/block-core.json |  4 +++-
>  qmp-commands.hx      |  2 +-
>  4 files changed, 18 insertions(+), 10 deletions(-)
> 

> -BlockStatsList *qmp_query_blockstats(Error **errp)
> +BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
> +                                     bool query_nodes,
> +                                     Error **errp)
>  {
>      BlockStatsList *head = NULL, **p_next = &head;
>      BlockDriverState *bs = NULL;
>  
> -     while ((bs = bdrv_next(bs))) {
> +    /* Just to be safe if query_nodes is not always intialized */

s/intialized/initialized/

> +    query_nodes = query_nodes && has_query_nodes;

If things aren't initialized (was true a while ago, but we recently
fixed that to ensure 0 initialization, although no one yet really relies
on the guarantee), then valgrind could complain of a branch on an uninit
memory location.  Idiomatically, this is usually written:

query_nodes = has_query_nodes && query_nodes;

to pacify valgrind if we hadn't zero-initialized; although logically,
the result is identical, so I don't care if you leave it.

> +++ b/qapi/block-core.json
> @@ -434,7 +434,9 @@
>  #
>  # Since: 0.14.0
>  ##
> -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> +{ 'command': 'query-blockstats',
> +  'data': {'*query-nodes': 'bool' },
> +  'returns': ['BlockStats'] }

Max correctly pointed out that this is missing documentation.

The idea looks sane; it will visit all named nodes (whether or not those
nodes are also reachable from named devices), and omit any unnamed nodes
(right now, libvirt would have to be taught to name nodes, or Jeff's
patch to auto-name nodes will avoid that problem).

Hmm, I wonder - if we are adding an optional parameter that controls
what to iterate over, should we also add an optional parameter that says
to limit the output to a given input name?  Then again, we don't have
very many existing query-* commands that filter, and at any rate, adding
such a filter should be its own patch.

-- 
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: 539 bytes --]