qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Reitz
Subject: Re: [PATCH 01/18] block: Make blk_{pread,pwrite}() return 0 on success
Date: Mon, 4 Jul 2022 15:52:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]