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

[Vladimir Sementsov-Ogievskiy]

22.04.2020 18:50, Stefan Hajnoczi wrote:
> 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.

Hmm, would be a bit strange to just add "bytes < 0" to the first "if" keeping "offset 
< 0" in the third.
And strange to keep "bytes > BDRV_REQUEST_MAX_BYTES" in the first, if add "bytes < 
0" to the third..

> > @@ -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;
>          }

You are right, I'll drop it.

--
Best regards,
Vladimir