Re: [PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success

[Greg Kurz <groug@kaod.org>]

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);