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

[Max Reitz <mreitz@redhat.com>]

On 2014-10-31 at 04:32, 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 | 11 ++++++++++-
>   qmp-commands.hx      |  2 +-
>   4 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a4d1a20..a0a50b9 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);
>       }
>   
>       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 initialized */
> +    query_nodes = has_query_nodes && 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 6b4b96c..db4e8e9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -430,11 +430,20 @@
>   #
>   # Query the @BlockStats for all virtual block devices.
>   #
> +# @query-nodes: #optional If true, the command will query all the block nodes
> +#               that have a node name, in a list which will include "parent"
> +#               information, but not "backing".
> +#               If false or omitted, the behavior is as before - query all the
> +#               device backends, recursively including their "parent" and #

Is there a reason for the # at the end of the line?

Fixable by a maintainer, though.

> +#               "backing". (Since 2.3)
> +#
>   # Returns: A list of @BlockStats for each virtual block devices.
>   #
>   # Since: 0.14.0
>   ##
> -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> +{ 'command': 'query-blockstats',
> +  'data': { '*query-nodes': 'bool' },
> +  '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,
>       },
>   

As written in the reply I forgot to give to v1, I'm still concerned some 
nodes may appear twice (named protocol BDS) and maybe we don't want 
that. But it should not hurt.

With the # removed:

Reviewed-by: Max Reitz <mreitz@redhat.com>