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

[Hanna Reitz]

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