Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters

[Stefan Hajnoczi]

On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -875,9 +875,9 @@ static bool coroutine_fn 
> bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>  }
>  
>  static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> -                                   size_t size)
> +                                   int64_t bytes)
>  {
> -    if (size > BDRV_REQUEST_MAX_BYTES) {
> +    if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) {
>          return -EIO;
>      }
>  
> @@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, 
> int64_t offset,
>          return -ENOMEDIUM;
>      }
>  
> -    if (offset < 0) {
> -        return -EIO;
> -    }
> -
>      return 0;
>  }

Moving this if statement was unnecessary.  I'm not sure if there are
cases where callers will now see EIO instead of ENOMEDIUM and change
their behavior :(.

More conservative code changes are easier to review and merge because
they are less risky.

> @@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>  }
>  
>  static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> -    int64_t offset, int bytes, BdrvRequestFlags flags)
> +    int64_t offset, int64_t bytes, BdrvRequestFlags flags)
>  {
>      BlockDriver *drv = bs->drv;
>      QEMUIOVector qiov;
> @@ -1773,7 +1770,7 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>      assert(max_write_zeroes >= bs->bl.request_alignment);
>  
>      while (bytes > 0 && !ret) {
> -        int num = bytes;
> +        int64_t num = bytes;
>  
>          /* Align request.  Block drivers can expect the "bulk" of the request
>           * to be aligned, and that unaligned requests do not cross cluster
> @@ -1799,6 +1796,7 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>          ret = -ENOTSUP;
>          /* First try the efficient write zeroes operation */
>          if (drv->bdrv_co_pwrite_zeroes) {
> +            assert(num <= INT_MAX);

max_write_zeroes already enforces this, so the assertion is always
false:

  int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
  ...
        /* limit request size */
        if (num > max_write_zeroes) {
            num = max_write_zeroes;
        }

signature.asc
Description: PGP signature