Re: [PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers

[Eric Blake]

On Thu, Sep 23, 2021 at 03:33:45PM -0500, Eric Blake wrote:
> > +++ b/block/nbd.c
> > @@ -1407,15 +1407,17 @@ static int nbd_client_co_pwritev(BlockDriverState 
> > *bs, int64_t offset,
> >  }
> >  
> >  static int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t 
> > offset,
> > -                                       int bytes, BdrvRequestFlags flags)
> > +                                       int64_t bytes, BdrvRequestFlags 
> > flags)
> >  {
> >      BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> >      NBDRequest request = {
> >          .type = NBD_CMD_WRITE_ZEROES,
> >          .from = offset,
> > -        .len = bytes,
> > +        .len = bytes,  /* .len is uint32_t actually */
> >      };
> >  
> > +    assert(bytes < UINT32_MAX); /* relay on max_pwrite_zeroes */
> 
> And again.  Here, you happen to get by with < because we clamped
> bl.max_pwrite_zeroes at BDRV_REQUEST_MAX_BYTES, which is INT_MAX
> rounded down.  But I had to check; whereas using <= would be less
> worrisome, even if we never get a request that large.

Whoops, I was reading a local patch of mine.  Upstream has merely:

    uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block);

    bs->bl.max_pdiscard = QEMU_ALIGN_DOWN(INT_MAX, min);
    bs->bl.max_pwrite_zeroes = max;

which is an even smaller limit than BDRV_REQUEST_MAX_BYTES (and
obviously one we're trying to raise).  But the point remains that
using <= rather than < will make it easier to review the code where we
raise the limits (either up to the 4G-1 limit of the current protocol,
or with protocol extensions to finally get to 64-bit requests).

> 
> If you agree with my analysis, I can make that change while preparing
> my pull request.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org