Re: [Qemu-devel] [PATCH v5 1/8] block: Add bdrv_get_block_status_above

[Fam Zheng <famz@redhat.com>]

On Tue, 05/26 11:22, Paolo Bonzini wrote:
> 
> 
> On 26/05/2015 05:36, Fam Zheng wrote:
> > Like bdrv_is_allocated_above, this function follows the backing chain until seeing
> > BDRV_BLOCK_ALLOCATED.  Base is not included.
> > 
> > Reimplement bdrv_is_allocated on top.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/io.c            | 53 ++++++++++++++++++++++++++++++++++++++++-----------
> >  include/block/block.h |  4 ++++
> >  2 files changed, 46 insertions(+), 11 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index e394d92..a0d9990 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1560,28 +1560,51 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >      return ret;
> >  }
> >  
> > -/* Coroutine wrapper for bdrv_get_block_status() */
> > -static void coroutine_fn bdrv_get_block_status_co_entry(void *opaque)
> > +static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
> > +        BlockDriverState *base,
> > +        int64_t sector_num,
> > +        int nb_sectors,
> > +        int *pnum)
> > +{
> > +    BlockDriverState *p;
> > +    int64_t ret;
> > +
> > +    assert(bs != base);
> > +    for (p = bs; p != base; p = p->backing_hd) {
> > +        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
> 
> Since pnum is an output parameter only, *pnum will be set by the last
> call in the loop.
> 
> This is not what bdrv_is_allocated_above does: you have to set *pnum
> (roughly) to the _smallest_ value returned by the calls.  Consider this
> case (base == NULL, bs->backing_hd->backing_hd == NULL):
> 
>                                  1         2         3
>                         123456789012345678901234567890
>   bs                    ...........AAAAAAAAAAAA.......
>   bs->backing_hd        ...............AAAAAAAAAAAAAAA
> 
> Your code would return *pnum == 15, but the right result is *pnum == 11.

Yes, this is the case I missed! Thanks for explaining!

Fam