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

[Peter Lieven <pl@kamp.de>]

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 <pbonzini@redhat.com>
>>>
>>> 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 <eblake@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>    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