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

[Kevin Wolf]

Am 02.03.2016 um 18:10 hat Max Reitz geschrieben:
> 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 <address@hidden>
> > ---
> >  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?

This is only possible since commit d9b7b057 ('block: Allow references
for backing files'), which wasn't in 2.5 yet.

Apart from that, you still get the information because that second BB is
enumerated as well. It's just not duplicated in the return value of a
different BB any more. I think we can consider that a bug fix.

Kevin

> If no:
> 
> Reviewed-by: Max Reitz <address@hidden>
>

pgprLgIXndVjp.pgp
Description: PGP signature