Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero

[Fam Zheng]

On Mon, 11/10 09:33, Markus Armbruster wrote:
> Fam Zheng <address@hidden> writes:
> 
> > In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX /
> > BDRV_SECTOR_SIZE, so a caller shouldn't exceed it.

I noticed this while testing unmap / zero write with scsi_debug:

# dd if=/dev/zero of=/tmp/a bs=1M count=32
# modprobe scsi_debug dev_size_mb=1024 lbpu=1
# qemu-img convert -t none /tmp/a /dev/sde
qemu-img: error writing zeroes at sector 0: Invalid argument

> 
> It's not obvious to me why we do that there.  iovec member iov_len is
> size_t, not int.

Deeper in the call stack we use int bytes everywhere:

static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
    BdrvRequestFlags flags);
static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
    int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
    BdrvRequestFlags flags);

static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
    BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
    int64_t align, QEMUIOVector *qiov, int flags)

So this is a easier way to fix the specific bug :)

> 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  block.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index dacd881..5513379 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2790,8 +2790,8 @@ int bdrv_make_zero(BlockDriverState *bs, 
> > BdrvRequestFlags flags)
> >          if (nb_sectors <= 0) {
> >              return 0;
> >          }
> > -        if (nb_sectors > INT_MAX) {
> > -            nb_sectors = INT_MAX;
> > +        if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> > +            nb_sectors = INT_MAX / BDRV_SECTOR_SIZE;
> >          }
> >          ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> >          if (ret < 0) {
> 
> Noticed while checkout out bdrv_get_block_status(): function comment of
> bdrv_co_get_block_status() claims it returns true/false.  It doesn't.
> 

Yeah, we should fix that.

Fam