| Message ID | 20240130002712.257815-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes() | expand |
On 30/1/24 01:27, Stefan Hajnoczi wrote: > The following expression is incorrect because blk_pread_nonzeroes() > deals in units of bytes, not sectors: > > bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS) > ^^^^^^^ > > BDRV_REQUEST_MAX_BYTES is the appropriate constant. > > Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image") > Cc: Xiang Zheng <zhengxiang9@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/block/block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/block/block.c b/hw/block/block.c > index 9f52ee6e72..ff503002aa 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf) > BlockDriverState *bs = blk_bs(blk); > > for (;;) { > - bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS); > + bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES); > if (bytes <= 0) { > return 0; > } Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Mon, Jan 29, 2024 at 07:27:12PM -0500, Stefan Hajnoczi wrote: > The following expression is incorrect because blk_pread_nonzeroes() > deals in units of bytes, not sectors: > > bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS) > ^^^^^^^ > > BDRV_REQUEST_MAX_BYTES is the appropriate constant. > > Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image") > Cc: Xiang Zheng <zhengxiang9@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/block/block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://gitlab.com/stefanha/qemu/commits/block Stefan
30.01.2024 03:27, Stefan Hajnoczi wrote: > The following expression is incorrect because blk_pread_nonzeroes() > deals in units of bytes, not sectors: > > bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS) > ^^^^^^^ > > BDRV_REQUEST_MAX_BYTES is the appropriate constant. > > Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image") > Cc: Xiang Zheng <zhengxiang9@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/block/block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/block/block.c b/hw/block/block.c > index 9f52ee6e72..ff503002aa 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf) > BlockDriverState *bs = blk_bs(blk); > > for (;;) { > - bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS); > + bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES); Hmm. This smells like a -stable material, but you know better not to Cc: qemu-stable@ for unrelated stuff... Is it not for stable? Thanks, /mjt
On Thu, 1 Feb 2024 at 06:37, Michael Tokarev <mjt@tls.msk.ru> wrote: > > 30.01.2024 03:27, Stefan Hajnoczi wrote: > > The following expression is incorrect because blk_pread_nonzeroes() > > deals in units of bytes, not sectors: > > > > bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS) > > ^^^^^^^ > > > > BDRV_REQUEST_MAX_BYTES is the appropriate constant. > > > > Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image") > > Cc: Xiang Zheng <zhengxiang9@huawei.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/block/block.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/block/block.c b/hw/block/block.c > > index 9f52ee6e72..ff503002aa 100644 > > --- a/hw/block/block.c > > +++ b/hw/block/block.c > > @@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf) > > BlockDriverState *bs = blk_bs(blk); > > > > for (;;) { > > - bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS); > > + bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES); > > Hmm. This smells like a -stable material, but you know better not > to Cc: qemu-stable@ for unrelated stuff... Is it not for stable? This is not a user-visible bug. The code still works with the smaller MAX_SECTORS value thanks to the loop. It doesn't hurt to include it in -stable but I also think it doesn't help :-). It's just an inconsistency in the code. Stefan
09.02.2024 00:21, Stefan Hajnoczi wrote: > On Thu, 1 Feb 2024 at 06:37, Michael Tokarev <mjt@tls.msk.ru> wrote: >>> for (;;) { >>> - bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS); >>> + bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES); >> >> Hmm. This smells like a -stable material, but you know better not >> to Cc: qemu-stable@ for unrelated stuff... Is it not for stable? > > This is not a user-visible bug. The code still works with the smaller > MAX_SECTORS value thanks to the loop. Yeah, that's my thoughts exactly. Also, most of the time, the cap will be `size' anyway, not MAX. Still thought I'd ask :) Thank you for the confirmation! /mjt > It doesn't hurt to include it in -stable but I also think it doesn't > help :-). It's just an inconsistency in the code.
diff --git a/hw/block/block.c b/hw/block/block.c index 9f52ee6e72..ff503002aa 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf) BlockDriverState *bs = blk_bs(blk); for (;;) { - bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS); + bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES); if (bytes <= 0) { return 0; }
The following expression is incorrect because blk_pread_nonzeroes() deals in units of bytes, not sectors: bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS) ^^^^^^^ BDRV_REQUEST_MAX_BYTES is the appropriate constant. Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image") Cc: Xiang Zheng <zhengxiang9@huawei.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)