| Message ID | 20220517113524.197910-2-afaria@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make block-backend-io.h API more consistent | expand |
On Tue, 17 May 2022 12:35:07 +0100 Alberto Faria <afaria@redhat.com> 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 <afaria@redhat.com> > --- > 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 <groug@kaod.org> > 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 2c00dddd80..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, &qiov, 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, &qiov, 0, flags); > - > - return ret < 0 ? ret : bytes; > + return blk_pwritev_part(blk, offset, bytes, &qiov, 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, &header, 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 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) { > 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 > @@ -103,7 +103,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, > { > SpaprNvram *nvram = spapr->nvram; > hwaddr offset, buffer, len; > - int alen; > + int ret; > void *membuf; > > if ((nargs != 3) || (nret != 2)) { > @@ -128,9 +128,9 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, > > membuf = cpu_physical_memory_map(buffer, &len, false); > > - alen = len; > + ret = 0; > if (nvram->blk) { > - alen = blk_pwrite(nvram->blk, offset, membuf, len, 0); > + ret = blk_pwrite(nvram->blk, offset, membuf, len, 0); > } > > assert(nvram->buf); > @@ -138,8 +138,8 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, > > cpu_physical_memory_unmap(membuf, len, 0, len); > > - rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS); > - rtas_st(rets, 1, (alen < 0) ? 0 : alen); > + rtas_st(rets, 0, (ret < 0) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS); > + rtas_st(rets, 1, (ret < 0) ? 0 : len); > } > > static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) > @@ -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) { > error_setg(errp, "can't read spapr-nvram contents"); > return; > } > diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c > index 83ecccca28..1fb748afef 100644 > --- a/hw/ppc/pnv_pnor.c > +++ b/hw/ppc/pnv_pnor.c > @@ -99,7 +99,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp) > > 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/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: > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 2f0d8ac25a..443f22c732 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -541,28 +541,34 @@ fail: > static int do_pread(BlockBackend *blk, char *buf, int64_t offset, > int64_t bytes, int64_t *total) > { > + int ret; > + > if (bytes > INT_MAX) { > return -ERANGE; > } > > - *total = blk_pread(blk, offset, (uint8_t *)buf, bytes); > - if (*total < 0) { > - return *total; > + ret = blk_pread(blk, offset, (uint8_t *)buf, bytes); > + if (ret < 0) { > + return ret; > } > + *total = bytes; > return 1; > } > > static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, > int64_t bytes, int flags, int64_t *total) > { > + int ret; > + > if (bytes > INT_MAX) { > return -ERANGE; > } > > - *total = blk_pwrite(blk, offset, (uint8_t *)buf, bytes, flags); > - if (*total < 0) { > - return *total; > + ret = blk_pwrite(blk, offset, (uint8_t *)buf, bytes, flags); > + if (ret < 0) { > + return ret; > } > + *total = bytes; > return 1; > } > > diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c > index a5c163af7e..3c1a3f64a2 100644 > --- a/tests/unit/test-block-iothread.c > +++ b/tests/unit/test-block-iothread.c > @@ -117,7 +117,7 @@ static void test_sync_op_blk_pread(BlockBackend *blk) > > /* Success */ > ret = blk_pread(blk, 0, buf, sizeof(buf)); > - g_assert_cmpint(ret, ==, 512); > + g_assert_cmpint(ret, ==, 0); > > /* Early error: Negative offset */ > ret = blk_pread(blk, -2, buf, sizeof(buf)); > @@ -131,7 +131,7 @@ static void test_sync_op_blk_pwrite(BlockBackend *blk) > > /* Success */ > ret = blk_pwrite(blk, 0, buf, sizeof(buf), 0); > - g_assert_cmpint(ret, ==, 512); > + g_assert_cmpint(ret, ==, 0); > > /* Early error: Negative offset */ > ret = blk_pwrite(blk, -2, buf, sizeof(buf), 0);
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 <afaria@redhat.com> > --- > +++ 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 <eblake@redhat.com>
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 <afaria@redhat.com> > --- > 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 <hreitz@redhat.com> 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 not to). Hanna
On Mon, Jul 4, 2022 at 2:52 PM Hanna Reitz <hreitz@redhat.com> 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
diff --git a/block.c b/block.c index 2c00dddd80..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, &qiov, 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, &qiov, 0, flags); - - return ret < 0 ? ret : bytes; + return blk_pwritev_part(blk, offset, bytes, &qiov, 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, &header, 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 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) { 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 @@ -103,7 +103,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, { SpaprNvram *nvram = spapr->nvram; hwaddr offset, buffer, len; - int alen; + int ret; void *membuf; if ((nargs != 3) || (nret != 2)) { @@ -128,9 +128,9 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, membuf = cpu_physical_memory_map(buffer, &len, false); - alen = len; + ret = 0; if (nvram->blk) { - alen = blk_pwrite(nvram->blk, offset, membuf, len, 0); + ret = blk_pwrite(nvram->blk, offset, membuf, len, 0); } assert(nvram->buf); @@ -138,8 +138,8 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr, cpu_physical_memory_unmap(membuf, len, 0, len); - rtas_st(rets, 0, (alen < len) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS); - rtas_st(rets, 1, (alen < 0) ? 0 : alen); + rtas_st(rets, 0, (ret < 0) ? RTAS_OUT_HW_ERROR : RTAS_OUT_SUCCESS); + rtas_st(rets, 1, (ret < 0) ? 0 : len); } static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) @@ -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) { error_setg(errp, "can't read spapr-nvram contents"); return; } diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c index 83ecccca28..1fb748afef 100644 --- a/hw/ppc/pnv_pnor.c +++ b/hw/ppc/pnv_pnor.c @@ -99,7 +99,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp) 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/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: diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 2f0d8ac25a..443f22c732 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -541,28 +541,34 @@ fail: static int do_pread(BlockBackend *blk, char *buf, int64_t offset, int64_t bytes, int64_t *total) { + int ret; + if (bytes > INT_MAX) { return -ERANGE; } - *total = blk_pread(blk, offset, (uint8_t *)buf, bytes); - if (*total < 0) { - return *total; + ret = blk_pread(blk, offset, (uint8_t *)buf, bytes); + if (ret < 0) { + return ret; } + *total = bytes; return 1; } static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, int64_t bytes, int flags, int64_t *total) { + int ret; + if (bytes > INT_MAX) { return -ERANGE; } - *total = blk_pwrite(blk, offset, (uint8_t *)buf, bytes, flags); - if (*total < 0) { - return *total; + ret = blk_pwrite(blk, offset, (uint8_t *)buf, bytes, flags); + if (ret < 0) { + return ret; } + *total = bytes; return 1; } diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index a5c163af7e..3c1a3f64a2 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -117,7 +117,7 @@ static void test_sync_op_blk_pread(BlockBackend *blk) /* Success */ ret = blk_pread(blk, 0, buf, sizeof(buf)); - g_assert_cmpint(ret, ==, 512); + g_assert_cmpint(ret, ==, 0); /* Early error: Negative offset */ ret = blk_pread(blk, -2, buf, sizeof(buf)); @@ -131,7 +131,7 @@ static void test_sync_op_blk_pwrite(BlockBackend *blk) /* Success */ ret = blk_pwrite(blk, 0, buf, sizeof(buf), 0); - g_assert_cmpint(ret, ==, 512); + g_assert_cmpint(ret, ==, 0); /* Early error: Negative offset */ ret = blk_pwrite(blk, -2, buf, sizeof(buf), 0);
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 <afaria@redhat.com> --- 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(-)