Re: [Qemu-devel] [PULL 30/42] block: define get_block_status return value

[Peter Lieven]

On 13.09.2013 10:26, Paolo Bonzini wrote:
> Il 13/09/2013 10:04, Peter Lieven ha scritto:
> > On 06.09.2013 17:39, Stefan Hajnoczi wrote:
> > > From: Paolo Bonzini <address@hidden>
> > >
> > > Define the return value of get_block_status.  Bits 0, 1, 2 and 9-62
> > > are valid; bit 63 (the sign bit) is reserved for errors.  Bits 3-8
> > > are left for future extensions.
> > >
> > > The return code is compatible with the old is_allocated API: if a driver
> > > only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to,
> > > clients of is_allocated will not have any change in behavior.  Still,
> > > we will return more precise information in the next patches and the
> > > new definition of bdrv_is_allocated is already prepared for this.
> > >
> > > Reviewed-by: Eric Blake <address@hidden>
> > > Signed-off-by: Paolo Bonzini <address@hidden>
> > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > ---
> > >    block.c               | 10 ++++++++--
> > >    include/block/block.h | 26 ++++++++++++++++++++++++++
> > >    2 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/block.c b/block.c
> > > index a5bfe74..97406ec 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -3093,7 +3093,7 @@ static int64_t coroutine_fn
> > > bdrv_co_get_block_status(BlockDriverState *bs,
> > >          if (!bs->drv->bdrv_co_get_block_status) {
> > >            *pnum = nb_sectors;
> > > -        return 1;
> > > +        return BDRV_BLOCK_DATA;
> > >        }
> > >          return bs->drv->bdrv_co_get_block_status(bs, sector_num,
> > > nb_sectors, pnum);
> > > @@ -3143,7 +3143,13 @@ int64_t bdrv_get_block_status(BlockDriverState
> > > *bs, int64_t sector_num,
> > >    int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t
> > > sector_num,
> > >                                       int nb_sectors, int *pnum)
> > >    {
> > > -    return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
> > > +    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors,
> > > pnum);
> > > +    if (ret < 0) {
> > sorry if I repeat myself.
> >
> > isn't here a *pnum = 0 missing?
> I think an earlier patch makes it unnecessary, because I'm checking the
> return status of bdrv_get_block_status and bdrv_is_allocated.  In any
> case the right place to put it would be in bdrv_get_block_status.
Ok, then I think for safety it should be added here:

@@ -3104,13 +3151,14 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,

     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
     if (ret < 0) {
+        *pnum = 0;
         return ret;
     }

because the new get_block_status api requires a ret < 0 if an error occurs. in 
case of an error
someone will write sth like ret = -EINVAL in the block driver. this not 
necessary implies *pnum = 0; the old
bdrv_is_allocated framework required a *pnum = 0 on error, but if someone (like 
me for iscsi)
creates bdrv_get_block_status from scratch will not explicity do this.

Peter