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 <f...@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
