Re: [PATCH 01/18] block: Make blk_{pread, pwrite}() return 0 on success
On Mon, Jul 4, 2022 at 2:52 PM Hanna Reitz wrote:
> There are a couple of places where you decided to replace “*len”
> variables that used to store the return value by a plain “ret”. That
> seems good to me, given how these functions no longer return length
> values, but you haven’t done so consistently. Below, I have noted
> places where this wasn’t done; I wonder why, but my R-b stands
> regardless of whether you change them too or not.
Thanks, this was just an oversight on my part. I'll fix it.
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 4cf4d2423d..9d98ef63ac 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -5120,30 +5120,27 @@ static int img_dd(int argc, char **argv)
> > in.buf = g_new(uint8_t, in.bsz);
> >
> > for (out_pos = 0; in_pos < size; block_count++) {
> > -int in_ret, out_ret;
> > +int bytes, in_ret, out_ret;
> >
> > -if (in_pos + in.bsz > size) {
> > -in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
> > -} else {
> > -in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
> > -}
> > +bytes = (in_pos + in.bsz > size) ? size - in_pos : in.bsz;
> > +
> > +in_ret = blk_pread(blk1, in_pos, in.buf, bytes);
> > if (in_ret < 0) {
> > error_report("error while reading from input image file: %s",
> >strerror(-in_ret));
> > ret = -1;
> > goto out;
> > }
> > -in_pos += in_ret;
> > -
> > -out_ret = blk_pwrite(blk2, out_pos, in.buf, in_ret, 0);
> > +in_pos += bytes;
> >
> > +out_ret = blk_pwrite(blk2, out_pos, in.buf, bytes, 0);
> > if (out_ret < 0) {
> > error_report("error while writing to output image file: %s",
> >strerror(-out_ret));
> > ret = -1;
> > goto out;
> > }
> > -out_pos += out_ret;
> > +out_pos += bytes;
> > }
> >
> > out:
>
> We could use this opportunity to drop in_ret and out_ret altogether (but
> we can also decide not to).
Dropping them sounds good to me.
Alberto
Re: [PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success
On 17.05.22 13:35, Alberto Faria wrote:
They currently return the value of their 'bytes' parameter on success.
Make them return 0 instead, for consistency with other I/O functions and
in preparation to implement them using generated_co_wrapper. This also
makes it clear that short reads/writes are not possible.
Signed-off-by: Alberto Faria
---
block.c | 8 +---
block/block-backend.c| 7 ++-
block/qcow.c | 6 +++---
hw/block/m25p80.c| 2 +-
hw/misc/mac_via.c| 2 +-
hw/misc/sifive_u_otp.c | 2 +-
hw/nvram/eeprom_at24c.c | 4 ++--
hw/nvram/spapr_nvram.c | 12 ++--
hw/ppc/pnv_pnor.c| 2 +-
qemu-img.c | 17 +++--
qemu-io-cmds.c | 18 --
tests/unit/test-block-iothread.c | 4 ++--
12 files changed, 43 insertions(+), 41 deletions(-)
Overall, looks good to me, so first of all:
Reviewed-by: Hanna Reitz
There are a couple of places where you decided to replace “*len”
variables that used to store the return value by a plain “ret”. That
seems good to me, given how these functions no longer return length
values, but you haven’t done so consistently. Below, I have noted
places where this wasn’t done; I wonder why, but my R-b stands
regardless of whether you change them too or not.
[...]
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 525e38ce93..0515d1818e 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -1030,7 +1030,7 @@ static void mos6522_q800_via1_realize(DeviceState *dev,
Error **errp)
}
len = blk_pread(v1s->blk, 0, v1s->PRAM, sizeof(v1s->PRAM));
-if (len != sizeof(v1s->PRAM)) {
+if (len < 0) {
We could use `ret` here instead.
error_setg(errp, "can't read PRAM contents");
return;
}
[...]
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600..84acd71f5a 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -65,7 +65,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
DPRINTK("clear\n");
if (ee->blk && ee->changed) {
int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
-if (len != ee->rsize) {
+if (len < 0) {
ERR(TYPE_AT24C_EE
" : failed to write backing file\n");
}
@@ -165,7 +165,7 @@ void at24c_eeprom_reset(DeviceState *state)
if (ee->blk) {
int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
-if (len != ee->rsize) {
+if (len < 0) {
We could rename `len` to `ret` in both of these hunks.
ERR(TYPE_AT24C_EE
" : Failed initial sync with backing file\n");
}
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 18b43be7f6..6000b945c3 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
[...]
@@ -181,7 +181,7 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error
**errp)
if (nvram->blk) {
int alen = blk_pread(nvram->blk, 0, nvram->buf, nvram->size);
-if (alen != nvram->size) {
+if (alen < 0) {
As above (and the other case in this file), might be nice to drop `alen`
here and just use `ret` instead.
error_setg(errp, "can't read spapr-nvram contents");
return;
}
[...]
diff --git a/qemu-img.c b/qemu-img.c
index 4cf4d2423d..9d98ef63ac 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5120,30 +5120,27 @@ static int img_dd(int argc, char **argv)
in.buf = g_new(uint8_t, in.bsz);
for (out_pos = 0; in_pos < size; block_count++) {
-int in_ret, out_ret;
+int bytes, in_ret, out_ret;
-if (in_pos + in.bsz > size) {
-in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
-} else {
-in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
-}
+bytes = (in_pos + in.bsz > size) ? size - in_pos : in.bsz;
+
+in_ret = blk_pread(blk1, in_pos, in.buf, bytes);
if (in_ret < 0) {
error_report("error while reading from input image file: %s",
strerror(-in_ret));
ret = -1;
goto out;
}
-in_pos += in_ret;
-
-out_ret = blk_pwrite(blk2, out_pos, in.buf, in_ret, 0);
+in_pos += bytes;
+out_ret = blk_pwrite(blk2, out_pos, in.buf, bytes, 0);
if (out_ret < 0) {
error_report("error while writing to output image file: %s",
strerror(-out_ret));
ret = -1;
goto out;
}
-out_pos += out_ret;
+out_pos += bytes;
}
out:
We could use this opportunity to drop in_ret and out_ret altogether (but
we can also decide
Re: [PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success
On Tue, May 17, 2022 at 12:35:07PM +0100, Alberto Faria wrote:
> They currently return the value of their 'bytes' parameter on success.
>
> Make them return 0 instead, for consistency with other I/O functions and
> in preparation to implement them using generated_co_wrapper. This also
> makes it clear that short reads/writes are not possible.
>
> Signed-off-by: Alberto Faria
> ---
> +++ b/qemu-img.c
> @@ -5120,30 +5120,27 @@ static int img_dd(int argc, char **argv)
> in.buf = g_new(uint8_t, in.bsz);
>
> for (out_pos = 0; in_pos < size; block_count++) {
in_pos, out_pos, and size are int64_t...
> -int in_ret, out_ret;
> +int bytes, in_ret, out_ret;
>
> -if (in_pos + in.bsz > size) {
> -in_ret = blk_pread(blk1, in_pos, in.buf, size - in_pos);
> -} else {
> -in_ret = blk_pread(blk1, in_pos, in.buf, in.bsz);
> -}
> +bytes = (in_pos + in.bsz > size) ? size - in_pos : in.bsz;
...but in.bsz is int, so declaring 'int bytes' appears safe.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success
On Tue, 17 May 2022 12:35:07 +0100
Alberto Faria wrote:
> They currently return the value of their 'bytes' parameter on success.
>
> Make them return 0 instead, for consistency with other I/O functions and
> in preparation to implement them using generated_co_wrapper. This also
> makes it clear that short reads/writes are not possible.
>
> Signed-off-by: Alberto Faria
> ---
> block.c | 8 +---
> block/block-backend.c| 7 ++-
> block/qcow.c | 6 +++---
> hw/block/m25p80.c| 2 +-
> hw/misc/mac_via.c| 2 +-
> hw/misc/sifive_u_otp.c | 2 +-
> hw/nvram/eeprom_at24c.c | 4 ++--
> hw/nvram/spapr_nvram.c | 12 ++--
> hw/ppc/pnv_pnor.c| 2 +-
For PPC and sPAPR parts
Reviewed-by: Greg Kurz
> qemu-img.c | 17 +++--
> qemu-io-cmds.c | 18 --
> tests/unit/test-block-iothread.c | 4 ++--
> 12 files changed, 43 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index 2c0080..0fd830e2e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1045,14 +1045,16 @@ static int find_image_format(BlockBackend *file,
> const char *filename,
> return ret;
> }
>
> -drv = bdrv_probe_all(buf, ret, filename);
> +drv = bdrv_probe_all(buf, sizeof(buf), filename);
> if (!drv) {
> error_setg(errp, "Could not determine image format: No compatible "
> "driver found");
> -ret = -ENOENT;
> +*pdrv = NULL;
> +return -ENOENT;
> }
> +
> *pdrv = drv;
> -return ret;
> +return 0;
> }
>
> /**
> diff --git a/block/block-backend.c b/block/block-backend.c
> index e0e1aff4b1..c1c367bf9e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1577,19 +1577,16 @@ int blk_pread(BlockBackend *blk, int64_t offset, void
> *buf, int bytes)
> ret = blk_do_preadv(blk, offset, bytes, , 0);
> blk_dec_in_flight(blk);
>
> -return ret < 0 ? ret : bytes;
> +return ret;
> }
>
> int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes,
> BdrvRequestFlags flags)
> {
> -int ret;
> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
> IO_OR_GS_CODE();
>
> -ret = blk_pwritev_part(blk, offset, bytes, , 0, flags);
> -
> -return ret < 0 ? ret : bytes;
> +return blk_pwritev_part(blk, offset, bytes, , 0, flags);
> }
>
> int64_t blk_getlength(BlockBackend *blk)
> diff --git a/block/qcow.c b/block/qcow.c
> index c646d6b16d..25a43353c1 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -891,14 +891,14 @@ static int coroutine_fn
> qcow_co_create(BlockdevCreateOptions *opts,
>
> /* write all the data */
> ret = blk_pwrite(qcow_blk, 0, , sizeof(header), 0);
> -if (ret != sizeof(header)) {
> +if (ret < 0) {
> goto exit;
> }
>
> if (qcow_opts->has_backing_file) {
> ret = blk_pwrite(qcow_blk, sizeof(header),
> qcow_opts->backing_file, backing_filename_len, 0);
> -if (ret != backing_filename_len) {
> +if (ret < 0) {
> goto exit;
> }
> }
> @@ -908,7 +908,7 @@ static int coroutine_fn
> qcow_co_create(BlockdevCreateOptions *opts,
> i++) {
> ret = blk_pwrite(qcow_blk, header_size + BDRV_SECTOR_SIZE * i,
> tmp, BDRV_SECTOR_SIZE, 0);
> -if (ret != BDRV_SECTOR_SIZE) {
> +if (ret < 0) {
> g_free(tmp);
> goto exit;
> }
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 7d3d8b12e0..bd58c07bb6 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1506,7 +1506,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error
> **errp)
> trace_m25p80_binding(s);
> s->storage = blk_blockalign(s->blk, s->size);
>
> -if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
> +if (blk_pread(s->blk, 0, s->storage, s->size) < 0) {
> error_setg(errp, "failed to read the initial flash content");
> return;
> }
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 525e38ce93..0515d1818e 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -1030,7 +1030,7 @@ static void mos6522_q800_via1_realize(DeviceState *dev,
> Error **errp)
> }
>
> len = blk_pread(v1s->blk, 0, v1s->PRAM, sizeof(v1s->PRAM));
> -if (len != sizeof(v1s->PRAM)) {
> +if (len < 0) {
> error_setg(errp, "can't read PRAM contents");
> return;
> }
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 6d5f84e6c2..535acde08b 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -240,7 +240,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error
> **errp)
>
[PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success
They currently return the value of their 'bytes' parameter on success.
Make them return 0 instead, for consistency with other I/O functions and
in preparation to implement them using generated_co_wrapper. This also
makes it clear that short reads/writes are not possible.
Signed-off-by: Alberto Faria
---
block.c | 8 +---
block/block-backend.c| 7 ++-
block/qcow.c | 6 +++---
hw/block/m25p80.c| 2 +-
hw/misc/mac_via.c| 2 +-
hw/misc/sifive_u_otp.c | 2 +-
hw/nvram/eeprom_at24c.c | 4 ++--
hw/nvram/spapr_nvram.c | 12 ++--
hw/ppc/pnv_pnor.c| 2 +-
qemu-img.c | 17 +++--
qemu-io-cmds.c | 18 --
tests/unit/test-block-iothread.c | 4 ++--
12 files changed, 43 insertions(+), 41 deletions(-)
diff --git a/block.c b/block.c
index 2c0080..0fd830e2e2 100644
--- a/block.c
+++ b/block.c
@@ -1045,14 +1045,16 @@ static int find_image_format(BlockBackend *file, const
char *filename,
return ret;
}
-drv = bdrv_probe_all(buf, ret, filename);
+drv = bdrv_probe_all(buf, sizeof(buf), filename);
if (!drv) {
error_setg(errp, "Could not determine image format: No compatible "
"driver found");
-ret = -ENOENT;
+*pdrv = NULL;
+return -ENOENT;
}
+
*pdrv = drv;
-return ret;
+return 0;
}
/**
diff --git a/block/block-backend.c b/block/block-backend.c
index e0e1aff4b1..c1c367bf9e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1577,19 +1577,16 @@ int blk_pread(BlockBackend *blk, int64_t offset, void
*buf, int bytes)
ret = blk_do_preadv(blk, offset, bytes, , 0);
blk_dec_in_flight(blk);
-return ret < 0 ? ret : bytes;
+return ret;
}
int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes,
BdrvRequestFlags flags)
{
-int ret;
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
IO_OR_GS_CODE();
-ret = blk_pwritev_part(blk, offset, bytes, , 0, flags);
-
-return ret < 0 ? ret : bytes;
+return blk_pwritev_part(blk, offset, bytes, , 0, flags);
}
int64_t blk_getlength(BlockBackend *blk)
diff --git a/block/qcow.c b/block/qcow.c
index c646d6b16d..25a43353c1 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -891,14 +891,14 @@ static int coroutine_fn
qcow_co_create(BlockdevCreateOptions *opts,
/* write all the data */
ret = blk_pwrite(qcow_blk, 0, , sizeof(header), 0);
-if (ret != sizeof(header)) {
+if (ret < 0) {
goto exit;
}
if (qcow_opts->has_backing_file) {
ret = blk_pwrite(qcow_blk, sizeof(header),
qcow_opts->backing_file, backing_filename_len, 0);
-if (ret != backing_filename_len) {
+if (ret < 0) {
goto exit;
}
}
@@ -908,7 +908,7 @@ static int coroutine_fn
qcow_co_create(BlockdevCreateOptions *opts,
i++) {
ret = blk_pwrite(qcow_blk, header_size + BDRV_SECTOR_SIZE * i,
tmp, BDRV_SECTOR_SIZE, 0);
-if (ret != BDRV_SECTOR_SIZE) {
+if (ret < 0) {
g_free(tmp);
goto exit;
}
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 7d3d8b12e0..bd58c07bb6 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1506,7 +1506,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
+if (blk_pread(s->blk, 0, s->storage, s->size) < 0) {
error_setg(errp, "failed to read the initial flash content");
return;
}
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 525e38ce93..0515d1818e 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -1030,7 +1030,7 @@ static void mos6522_q800_via1_realize(DeviceState *dev,
Error **errp)
}
len = blk_pread(v1s->blk, 0, v1s->PRAM, sizeof(v1s->PRAM));
-if (len != sizeof(v1s->PRAM)) {
+if (len < 0) {
error_setg(errp, "can't read PRAM contents");
return;
}
diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 6d5f84e6c2..535acde08b 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -240,7 +240,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error
**errp)
return;
}
-if (blk_pread(s->blk, 0, s->fuse, filesize) != filesize) {
+if (blk_pread(s->blk, 0, s->fuse, filesize) < 0) {
error_setg(errp, "failed to read the initial flash content");
return;
}
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600..84acd71f5a
