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

[Max Reitz <mreitz@redhat.com>]

On 2014-10-29 at 06:04, 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(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a4d1a20..e26033e 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -322,7 +322,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
>       qapi_free_BlockInfo(info);
>   }
>   
> -static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
> +static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
> +                                    bool query_backing)
>   {
>       BlockStats *s;
>   
> @@ -352,12 +353,12 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
>   
>       if (bs->file) {
>           s->has_parent = true;
> -        s->parent = bdrv_query_stats(bs->file);
> +        s->parent = bdrv_query_stats(bs->file, query_backing);
>       }
>   
> -    if (bs->backing_hd) {
> +    if (query_backing && bs->backing_hd) {
>           s->has_backing = true;
> -        s->backing = bdrv_query_stats(bs->backing_hd);
> +        s->backing = bdrv_query_stats(bs->backing_hd, query_backing);
>       }

Is there a specific reason why you're not querying the backing chain but 
still recurse to bs->file?

>       return s;
> @@ -388,17 +389,22 @@ BlockInfoList *qmp_query_block(Error **errp)
>       return NULL;
>   }
>   
> -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 */
> +    query_nodes = query_nodes && has_query_nodes;
> +
> +    while ((bs = query_nodes ? bdrv_next_node(bs) : bdrv_next(bs))) {
>           BlockStatsList *info = g_malloc0(sizeof(*info));
>           AioContext *ctx = bdrv_get_aio_context(bs);
>   
>           aio_context_acquire(ctx);
> -        info->value = bdrv_query_stats(bs);
> +        info->value = bdrv_query_stats(bs, !query_nodes);
>           aio_context_release(ctx);
>   
>           *p_next = info;
> diff --git a/hmp.c b/hmp.c
> index 63d7686..94b27df 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -403,7 +403,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>   {
>       BlockStatsList *stats_list, *stats;
>   
> -    stats_list = qmp_query_blockstats(NULL);
> +    stats_list = qmp_query_blockstats(false, false, NULL);
>   
>       for (stats = stats_list; stats; stats = stats->next) {
>           if (!stats->value->has_device) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c4beb17..a662137 100644
> --- a/qapi/block-core.json
> +++ 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' },

No space after the opening shwoopy bracket (took that from Wikipedia, I 
love it), but a space before the closing one.

Also, the new parameter should be documented, I think (and the 
documentation should include that the backing chain is not queried 
recursively).

Max

> +  'returns': ['BlockStats'] }
>   
>   ##
>   # @BlockdevOnError:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..44bfb9e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2347,7 +2347,7 @@ EQMP
>   
>       {
>           .name       = "query-blockstats",
> -        .args_type  = "",
> +        .args_type  = "query-nodes:b?",
>           .mhandler.cmd_new = qmp_marshal_input_query_blockstats,
>       },
>