Re: [Qemu-devel] [PATCH 3/3] block/qapi: Include empty drives in query-blockstats

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2930 bytes --]

On 26.02.2016 21:22, Kevin Wolf wrote:
> Since commit 5ec18f8c, query-blockstats didn't return the statistics of
> drives without media any more because such drives have only a BB now,
> but not a BDS any more.
> 
> This patch fixes the regression so that query-blockstats iterates over
> BBs by default and empty drives are displayed again.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 31ae879..6a4869a 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -355,7 +355,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(BlockBackend *blk,
> +                                    const BlockDriverState *bs,
>                                      bool query_backing);
>  
>  static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
> @@ -363,6 +364,9 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
>      BlockAcctStats *stats = blk_get_stats(blk);
>      BlockAcctTimedStats *ts = NULL;
>  
> +    s->has_device = true;
> +    s->device = g_strdup(blk_name(blk));
> +
>      s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
>      s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
>      s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
> @@ -428,11 +432,6 @@ static void bdrv_query_blk_stats(BlockStats *s, BlockBackend *blk)
>  static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
>                                   bool query_backing)
>  {
> -    if (bdrv_get_device_name(bs)[0]) {
> -        s->has_device = true;
> -        s->device = g_strdup(bdrv_get_device_name(bs));
> -    }
> -
>      if (bdrv_get_node_name(bs)[0]) {
>          s->has_node_name = true;
>          s->node_name = g_strdup(bdrv_get_node_name(bs));
> @@ -442,17 +441,18 @@ static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
>  
>      if (bs->file) {
>          s->has_parent = true;
> -        s->parent = bdrv_query_stats(bs->file->bs, query_backing);
> +        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
>      }
>  
>      if (query_backing && bs->backing) {
>          s->has_backing = true;
> -        s->backing = bdrv_query_stats(bs->backing->bs, query_backing);
> +        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);

This breaks my very important use case of having BBs on backing BDSs!!!

Kidding aside: Maybe someone actually wants to do this? Should we feel
bad for breaking query-blockstats for those BBs?

If no:

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


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