Re: [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv_get_block_status()

[Stefan Hajnoczi <stefanha@redhat.com>]

On Tue, Oct 03, 2017 at 08:43:44PM -0500, Eric Blake wrote:
> Handle a 0-length block status request up front, with a uniform
> return value claiming the area is not allocated.
> 
> Most callers don't pass a length of 0 to bdrv_get_block_status()
> and friends; but it definitely happens with a 0-length read when
> copy-on-read is enabled.  While we could audit all callers to
> ensure that they never make a 0-length request, and then assert
> that fact, it was just as easy to fix things to always report
> success (as long as the callers are careful to not go into an
> infinite loop).  However, we had inconsistent behavior on whether
> the status is reported as allocated or defers to the backing
> layer, depending on what callbacks the driver implements, and
> possibly wasting quite a few CPU cycles to get to that answer.
> Consistently reporting unallocated up front doesn't really hurt
> anything, and makes it easier both for callers (0-length requests
> now have well-defined behavior) and for drivers (drivers don't
> have to deal with 0-length requests).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: new patch
> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index e0f904583f..1f5baac41d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1773,9 +1773,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          return total_sectors;
>      }
> 
> -    if (sector_num >= total_sectors) {
> +    if (sector_num >= total_sectors || !nb_sectors) {
>          *pnum = 0;
> -        return BDRV_BLOCK_EOF;
> +        return sector_num >= total_sectors ? BDRV_BLOCK_EOF : 0;
>      }

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

If you respin, please consider using a separate if statement to make the
code clearer:

  if (!nb_sectors) {
      *pnum = 0;
      return 0;
  }
  if (sector_num >= total_sectors) {
      *pnum = 0;
      return BDRV_BLOCK_EOF;
  }