* [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero @ 2014-11-10 7:07 Fam Zheng 2014-11-10 8:33 ` Markus Armbruster 2014-11-13 11:06 ` Stefan Hajnoczi 0 siblings, 2 replies; 5+ messages in thread From: Fam Zheng @ 2014-11-10 7:07 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX / BDRV_SECTOR_SIZE, so a caller shouldn't exceed it. Signed-off-by: Fam Zheng <famz@redhat.com> --- 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) { -- 1.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero 2014-11-10 7:07 [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero Fam Zheng @ 2014-11-10 8:33 ` Markus Armbruster 2014-11-10 9:02 ` Fam Zheng 2014-11-13 11:06 ` Stefan Hajnoczi 1 sibling, 1 reply; 5+ messages in thread From: Markus Armbruster @ 2014-11-10 8:33 UTC (permalink / raw) To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi Fam Zheng <famz@redhat.com> writes: > In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX / > BDRV_SECTOR_SIZE, so a caller shouldn't exceed it. It's not obvious to me why we do that there. iovec member iov_len is size_t, not int. > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero 2014-11-10 8:33 ` Markus Armbruster @ 2014-11-10 9:02 ` Fam Zheng 2014-11-10 10:07 ` Markus Armbruster 0 siblings, 1 reply; 5+ messages in thread From: Fam Zheng @ 2014-11-10 9:02 UTC (permalink / raw) To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi On Mon, 11/10 09:33, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> 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 <famz@redhat.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero 2014-11-10 9:02 ` Fam Zheng @ 2014-11-10 10:07 ` Markus Armbruster 0 siblings, 0 replies; 5+ messages in thread From: Markus Armbruster @ 2014-11-10 10:07 UTC (permalink / raw) To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi Fam Zheng <famz@redhat.com> writes: > On Mon, 11/10 09:33, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> 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 :) Okay, that's enough for my R-by. Aside: using bytes makes sense, using int not so much. Reviewed-by: Markus Armbruster <armbru@redhat.com> [...] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero 2014-11-10 7:07 [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero Fam Zheng 2014-11-10 8:33 ` Markus Armbruster @ 2014-11-13 11:06 ` Stefan Hajnoczi 1 sibling, 0 replies; 5+ messages in thread From: Stefan Hajnoczi @ 2014-11-13 11:06 UTC (permalink / raw) To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi [-- Attachment #1: Type: text/plain, Size: 401 bytes --] On Mon, Nov 10, 2014 at 03:07:44PM +0800, Fam Zheng wrote: > In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX / > BDRV_SECTOR_SIZE, so a caller shouldn't exceed it. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan [-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-13 11:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-10 7:07 [Qemu-devel] [PATCH] block: Fix max nb_sectors in bdrv_make_zero Fam Zheng 2014-11-10 8:33 ` Markus Armbruster 2014-11-10 9:02 ` Fam Zheng 2014-11-10 10:07 ` Markus Armbruster 2014-11-13 11:06 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).