[Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[Alberto Garcia]

Hi,

this API only had a few users left so it can be easily removed.

Regards,

Berto

Alberto Garcia (5):
  qcow2: Replace bdrv_write() with bdrv_pwrite()
  vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
  vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
  block: Remove bdrv_read() and bdrv_write()
  qcow2: Remove BDRVQcow2State.cluster_sectors

 block/io.c             | 36 ------------------------------------
 block/qcow2-refcount.c |  4 ++--
 block/qcow2.c          |  1 -
 block/qcow2.h          |  1 -
 block/vdi.c            | 11 ++++++-----
 block/vvfat.c          | 10 ++++++----
 include/block/block.h  |  4 ----
 7 files changed, 14 insertions(+), 53 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[Alberto Garcia]

Hi,

this API only had a few users left so it can be easily removed.

Regards,

Berto

Alberto Garcia (5):
  qcow2: Replace bdrv_write() with bdrv_pwrite()
  vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
  vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
  block: Remove bdrv_read() and bdrv_write()
  qcow2: Remove BDRVQcow2State.cluster_sectors

 block/io.c             | 36 ------------------------------------
 block/qcow2-refcount.c |  4 ++--
 block/qcow2.c          |  1 -
 block/qcow2.h          |  1 -
 block/vdi.c            | 11 ++++++-----
 block/vvfat.c          | 10 ++++++----
 include/block/block.h  |  4 ----
 7 files changed, 14 insertions(+), 53 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite()

[Alberto Garcia]

There's only one bdrv_write() call left in the qcow2 code, and it can
be trivially replaced with the byte-based bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-refcount.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..83f66eed7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2409,8 +2409,8 @@ write_refblocks:
         on_disk_refblock = (void *)((char *) *refcount_table +
                                     refblock_index * s->cluster_size);
 
-        ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
-                         on_disk_refblock, s->cluster_sectors);
+        ret = bdrv_pwrite(bs->file, refblock_offset, on_disk_refblock,
+                          s->cluster_size);
         if (ret < 0) {
             fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
             goto fail;
-- 
2.11.0

[Qemu-devel] [PATCH 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite()

[Alberto Garcia]

There's only one bdrv_write() call left in the qcow2 code, and it can
be trivially replaced with the byte-based bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-refcount.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..83f66eed7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2409,8 +2409,8 @@ write_refblocks:
         on_disk_refblock = (void *)((char *) *refcount_table +
                                     refblock_index * s->cluster_size);
 
-        ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
-                         on_disk_refblock, s->cluster_sectors);
+        ret = bdrv_pwrite(bs->file, refblock_offset, on_disk_refblock,
+                          s->cluster_size);
         if (ret < 0) {
             fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
             goto fail;
-- 
2.11.0

[Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Alberto Garcia]

There's only a couple of bdrv_read() and bdrv_write() calls left in
the vdi code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/vdi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index e1c42ad732..8d849b2754 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -384,7 +384,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
 
     logout("\n");
 
-    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
+    ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
     if (ret < 0) {
         goto fail;
     }
@@ -484,8 +484,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
-                    bmap_size);
+    ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
+                     bmap_size * SECTOR_SIZE);
     if (ret < 0) {
         goto fail_free_bmap;
     }
@@ -704,7 +704,7 @@ nonallocating_write:
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_write(bs->file, 0, block, 1);
+        ret = bdrv_pwrite(bs->file, 0, block, sizeof(*block));
         g_free(block);
         block = NULL;
 
@@ -722,7 +722,8 @@ nonallocating_write:
         base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
-        ret = bdrv_write(bs->file, offset, base, n_sectors);
+        ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
+                          n_sectors * SECTOR_SIZE);
     }
 
     return ret;
-- 
2.11.0

[Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Alberto Garcia]

There's only a couple of bdrv_read() and bdrv_write() calls left in
the vdi code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/vdi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index e1c42ad732..8d849b2754 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -384,7 +384,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
 
     logout("\n");
 
-    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
+    ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
     if (ret < 0) {
         goto fail;
     }
@@ -484,8 +484,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
-                    bmap_size);
+    ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
+                     bmap_size * SECTOR_SIZE);
     if (ret < 0) {
         goto fail_free_bmap;
     }
@@ -704,7 +704,7 @@ nonallocating_write:
         assert(VDI_IS_ALLOCATED(bmap_first));
         *header = s->header;
         vdi_header_to_le(header);
-        ret = bdrv_write(bs->file, 0, block, 1);
+        ret = bdrv_pwrite(bs->file, 0, block, sizeof(*block));
         g_free(block);
         block = NULL;
 
@@ -722,7 +722,8 @@ nonallocating_write:
         base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
-        ret = bdrv_write(bs->file, offset, base, n_sectors);
+        ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
+                          n_sectors * SECTOR_SIZE);
     }
 
     return ret;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Kevin Wolf]

Am 29.04.2019 um 20:42 hat Alberto Garcia geschrieben:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vdi code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/vdi.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index e1c42ad732..8d849b2754 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -384,7 +384,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      logout("\n");
>  
> -    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
> +    ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));

Maybe worth adding this after the VdiHeader declaration?

QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);

>      if (ret < 0) {
>          goto fail;
>      }
> @@ -484,8 +484,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
> -                    bmap_size);
> +    ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
> +                     bmap_size * SECTOR_SIZE);
>      if (ret < 0) {
>          goto fail_free_bmap;
>      }
> @@ -704,7 +704,7 @@ nonallocating_write:
>          assert(VDI_IS_ALLOCATED(bmap_first));
>          *header = s->header;
>          vdi_header_to_le(header);
> -        ret = bdrv_write(bs->file, 0, block, 1);
> +        ret = bdrv_pwrite(bs->file, 0, block, sizeof(*block));

block is uint8_t*, so sizeof(*block) == 1. This is not what you want.
(qemu-iotests catches this pretty early.)

>          g_free(block);
>          block = NULL;
>  
> @@ -722,7 +722,8 @@ nonallocating_write:
>          base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
>          logout("will write %u block map sectors starting from entry %u\n",
>                 n_sectors, bmap_first);
> -        ret = bdrv_write(bs->file, offset, base, n_sectors);
> +        ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
> +                          n_sectors * SECTOR_SIZE);
>      }
>  
>      return ret;

Let's avoid returning a non-zero positive number in the success case
here. bdrv_driver_pwritev() checks ret == 0 for BDRV_REQ_FUA emulation,
and its callers don't expect positive results either.

Maybe we should actually assert() in bdrv_driver_preadv/pwritev() that
the result is <= 0.

Kevin

Re: [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Kevin Wolf]

Am 29.04.2019 um 20:42 hat Alberto Garcia geschrieben:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vdi code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/vdi.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index e1c42ad732..8d849b2754 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -384,7 +384,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      logout("\n");
>  
> -    ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
> +    ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));

Maybe worth adding this after the VdiHeader declaration?

QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);

>      if (ret < 0) {
>          goto fail;
>      }
> @@ -484,8 +484,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
> -                    bmap_size);
> +    ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
> +                     bmap_size * SECTOR_SIZE);
>      if (ret < 0) {
>          goto fail_free_bmap;
>      }
> @@ -704,7 +704,7 @@ nonallocating_write:
>          assert(VDI_IS_ALLOCATED(bmap_first));
>          *header = s->header;
>          vdi_header_to_le(header);
> -        ret = bdrv_write(bs->file, 0, block, 1);
> +        ret = bdrv_pwrite(bs->file, 0, block, sizeof(*block));

block is uint8_t*, so sizeof(*block) == 1. This is not what you want.
(qemu-iotests catches this pretty early.)

>          g_free(block);
>          block = NULL;
>  
> @@ -722,7 +722,8 @@ nonallocating_write:
>          base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
>          logout("will write %u block map sectors starting from entry %u\n",
>                 n_sectors, bmap_first);
> -        ret = bdrv_write(bs->file, offset, base, n_sectors);
> +        ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
> +                          n_sectors * SECTOR_SIZE);
>      }
>  
>      return ret;

Let's avoid returning a non-zero positive number in the success case
here. bdrv_driver_pwritev() checks ret == 0 for BDRV_REQ_FUA emulation,
and its callers don't expect positive results either.

Maybe we should actually assert() in bdrv_driver_preadv/pwritev() that
the result is <= 0.

Kevin

[Qemu-devel] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Alberto Garcia]

There's only a couple of bdrv_read() and bdrv_write() calls left in
the vvfat code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/vvfat.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 5f66787890..35c7e2761f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
                 DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
                              " allocated\n", sector_num,
                              n >> BDRV_SECTOR_BITS));
-                if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
-                              n >> BDRV_SECTOR_BITS)) {
+                if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
+                               buf + i * 0x200, n)) {
                     return -1;
                 }
                 i += (n >> BDRV_SECTOR_BITS) - 1;
@@ -1983,7 +1983,8 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
                         if (res) {
                             return -1;
                         }
-                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
+                        res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+                                          s->cluster_buffer, BDRV_SECTOR_SIZE);
                         if (res) {
                             return -2;
                         }
@@ -3050,7 +3051,8 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+    ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
+                      nb_sectors * BDRV_SECTOR_SIZE);
     if (ret < 0) {
         fprintf(stderr, "Error writing to qcow backend\n");
         return ret;
-- 
2.11.0

[Qemu-devel] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Alberto Garcia]

There's only a couple of bdrv_read() and bdrv_write() calls left in
the vvfat code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/vvfat.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 5f66787890..35c7e2761f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
                 DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
                              " allocated\n", sector_num,
                              n >> BDRV_SECTOR_BITS));
-                if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
-                              n >> BDRV_SECTOR_BITS)) {
+                if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
+                               buf + i * 0x200, n)) {
                     return -1;
                 }
                 i += (n >> BDRV_SECTOR_BITS) - 1;
@@ -1983,7 +1983,8 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
                         if (res) {
                             return -1;
                         }
-                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
+                        res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+                                          s->cluster_buffer, BDRV_SECTOR_SIZE);
                         if (res) {
                             return -2;
                         }
@@ -3050,7 +3051,8 @@ DLOG(checkpoint());
      * Use qcow backend. Commit later.
      */
 DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
-    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+    ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
+                      nb_sectors * BDRV_SECTOR_SIZE);
     if (ret < 0) {
         fprintf(stderr, "Error writing to qcow backend\n");
         return ret;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Kevin Wolf]

Am 29.04.2019 um 20:42 hat Alberto Garcia geschrieben:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vvfat code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/vvfat.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 5f66787890..35c7e2761f 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
>                  DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>                               " allocated\n", sector_num,
>                               n >> BDRV_SECTOR_BITS));
> -                if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
> -                              n >> BDRV_SECTOR_BITS)) {
> +                if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
> +                               buf + i * 0x200, n)) {

bdrv_pread() returns a positive number of bytes in the success case, so
this error check is wrong. (No real reason why it couldn't return 0, but
we would have to check and possibly update all callers, so we never did
that.)

>                      return -1;
>                  }
>                  i += (n >> BDRV_SECTOR_BITS) - 1;
> @@ -1983,7 +1983,8 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>                          if (res) {
>                              return -1;
>                          }
> -                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
> +                        res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
> +                                          s->cluster_buffer, BDRV_SECTOR_SIZE);
>                          if (res) {

Same here, this needs to be res < 0 now.

>                              return -2;
>                          }
> @@ -3050,7 +3051,8 @@ DLOG(checkpoint());
>       * Use qcow backend. Commit later.
>       */
>  DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
> -    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
> +    ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
> +                      nb_sectors * BDRV_SECTOR_SIZE);
>      if (ret < 0) {

This one is already correct.

>          fprintf(stderr, "Error writing to qcow backend\n");
>          return ret;

Kevin

Re: [Qemu-devel] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Kevin Wolf]

Am 29.04.2019 um 20:42 hat Alberto Garcia geschrieben:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vvfat code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/vvfat.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 5f66787890..35c7e2761f 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
>                  DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>                               " allocated\n", sector_num,
>                               n >> BDRV_SECTOR_BITS));
> -                if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
> -                              n >> BDRV_SECTOR_BITS)) {
> +                if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
> +                               buf + i * 0x200, n)) {

bdrv_pread() returns a positive number of bytes in the success case, so
this error check is wrong. (No real reason why it couldn't return 0, but
we would have to check and possibly update all callers, so we never did
that.)

>                      return -1;
>                  }
>                  i += (n >> BDRV_SECTOR_BITS) - 1;
> @@ -1983,7 +1983,8 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>                          if (res) {
>                              return -1;
>                          }
> -                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
> +                        res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
> +                                          s->cluster_buffer, BDRV_SECTOR_SIZE);
>                          if (res) {

Same here, this needs to be res < 0 now.

>                              return -2;
>                          }
> @@ -3050,7 +3051,8 @@ DLOG(checkpoint());
>       * Use qcow backend. Commit later.
>       */
>  DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
> -    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
> +    ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
> +                      nb_sectors * BDRV_SECTOR_SIZE);
>      if (ret < 0) {

This one is already correct.

>          fprintf(stderr, "Error writing to qcow backend\n");
>          return ret;

Kevin

[Qemu-devel] [PATCH 4/5] block: Remove bdrv_read() and bdrv_write()

[Alberto Garcia]

No one is using these functions anymore, all callers have switched to
the byte-based bdrv_pread() and bdrv_pwrite()

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/io.c            | 36 ------------------------------------
 include/block/block.h |  4 ----
 2 files changed, 40 deletions(-)

diff --git a/block/io.c b/block/io.c
index dfc153b8d8..adf759171a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -837,42 +837,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
     return rwco.ret;
 }
 
-/*
- * Process a synchronous request using coroutines
- */
-static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
-                      int nb_sectors, bool is_write, BdrvRequestFlags flags)
-{
-    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
-                                            nb_sectors * BDRV_SECTOR_SIZE);
-
-    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
-        return -EINVAL;
-    }
-
-    return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
-                        &qiov, is_write, flags);
-}
-
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BdrvChild *child, int64_t sector_num,
-              uint8_t *buf, int nb_sectors)
-{
-    return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
-}
-
-/* Return < 0 if error. Important errors are:
-  -EIO         generic I/O error (may happen for all errors)
-  -ENOMEDIUM   No media inserted.
-  -EINVAL      Invalid sector number or nb_sectors
-  -EACCES      Trying to write a read-only device
-*/
-int bdrv_write(BdrvChild *child, int64_t sector_num,
-               const uint8_t *buf, int nb_sectors)
-{
-    return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
-}
-
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
diff --git a/include/block/block.h b/include/block/block.h
index c7a26199aa..5e2b98b0ee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,10 +316,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BdrvChild *child, int64_t sector_num,
-              uint8_t *buf, int nb_sectors);
-int bdrv_write(BdrvChild *child, int64_t sector_num,
-               const uint8_t *buf, int nb_sectors);
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags);
 int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-- 
2.11.0

[Qemu-devel] [PATCH 4/5] block: Remove bdrv_read() and bdrv_write()

[Alberto Garcia]

No one is using these functions anymore, all callers have switched to
the byte-based bdrv_pread() and bdrv_pwrite()

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/io.c            | 36 ------------------------------------
 include/block/block.h |  4 ----
 2 files changed, 40 deletions(-)

diff --git a/block/io.c b/block/io.c
index dfc153b8d8..adf759171a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -837,42 +837,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
     return rwco.ret;
 }
 
-/*
- * Process a synchronous request using coroutines
- */
-static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
-                      int nb_sectors, bool is_write, BdrvRequestFlags flags)
-{
-    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
-                                            nb_sectors * BDRV_SECTOR_SIZE);
-
-    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
-        return -EINVAL;
-    }
-
-    return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
-                        &qiov, is_write, flags);
-}
-
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BdrvChild *child, int64_t sector_num,
-              uint8_t *buf, int nb_sectors)
-{
-    return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
-}
-
-/* Return < 0 if error. Important errors are:
-  -EIO         generic I/O error (may happen for all errors)
-  -ENOMEDIUM   No media inserted.
-  -EINVAL      Invalid sector number or nb_sectors
-  -EACCES      Trying to write a read-only device
-*/
-int bdrv_write(BdrvChild *child, int64_t sector_num,
-               const uint8_t *buf, int nb_sectors)
-{
-    return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
-}
-
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
diff --git a/include/block/block.h b/include/block/block.h
index c7a26199aa..5e2b98b0ee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,10 +316,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
 void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BdrvChild *child, int64_t sector_num,
-              uint8_t *buf, int nb_sectors);
-int bdrv_write(BdrvChild *child, int64_t sector_num,
-               const uint8_t *buf, int nb_sectors);
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags);
 int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-- 
2.11.0

[Qemu-devel] [PATCH 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors

[Alberto Garcia]

The last user of this field disappeared when we replace the
sector-based bdrv_write() with the byte-based bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 1 -
 block/qcow2.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3ace3b2209..3a3240fcca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1259,7 +1259,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     s->cluster_bits = header.cluster_bits;
     s->cluster_size = 1 << s->cluster_bits;
-    s->cluster_sectors = 1 << (s->cluster_bits - BDRV_SECTOR_BITS);
 
     /* Initialise version 3 header fields */
     if (header.version == 2) {
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..e62508d1ce 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -266,7 +266,6 @@ typedef struct Qcow2BitmapHeaderExt {
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
-    int cluster_sectors;
     int l2_slice_size;
     int l2_bits;
     int l2_size;
-- 
2.11.0

[Qemu-devel] [PATCH 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors

[Alberto Garcia]

The last user of this field disappeared when we replace the
sector-based bdrv_write() with the byte-based bdrv_pwrite().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 1 -
 block/qcow2.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3ace3b2209..3a3240fcca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1259,7 +1259,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     s->cluster_bits = header.cluster_bits;
     s->cluster_size = 1 << s->cluster_bits;
-    s->cluster_sectors = 1 << (s->cluster_bits - BDRV_SECTOR_BITS);
 
     /* Initialise version 3 header fields */
     if (header.version == 2) {
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..e62508d1ce 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -266,7 +266,6 @@ typedef struct Qcow2BitmapHeaderExt {
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
-    int cluster_sectors;
     int l2_slice_size;
     int l2_bits;
     int l2_size;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On 4/29/19 1:42 PM, Alberto Garcia wrote:
> Hi,
> 
> this API only had a few users left so it can be easily removed.

Sounds very similar to my earlier attempt at the same:

https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02769.html

> 
> Regards,
> 
> Berto
> 
> Alberto Garcia (5):
>   qcow2: Replace bdrv_write() with bdrv_pwrite()
>   vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
>   vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
>   block: Remove bdrv_read() and bdrv_write()
>   qcow2: Remove BDRVQcow2State.cluster_sectors
> 
>  block/io.c             | 36 ------------------------------------
>  block/qcow2-refcount.c |  4 ++--
>  block/qcow2.c          |  1 -
>  block/qcow2.h          |  1 -
>  block/vdi.c            | 11 ++++++-----
>  block/vvfat.c          | 10 ++++++----
>  include/block/block.h  |  4 ----
>  7 files changed, 14 insertions(+), 53 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On 4/29/19 1:42 PM, Alberto Garcia wrote:
> Hi,
> 
> this API only had a few users left so it can be easily removed.

Sounds very similar to my earlier attempt at the same:

https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02769.html

> 
> Regards,
> 
> Berto
> 
> Alberto Garcia (5):
>   qcow2: Replace bdrv_write() with bdrv_pwrite()
>   vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
>   vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
>   block: Remove bdrv_read() and bdrv_write()
>   qcow2: Remove BDRVQcow2State.cluster_sectors
> 
>  block/io.c             | 36 ------------------------------------
>  block/qcow2-refcount.c |  4 ++--
>  block/qcow2.c          |  1 -
>  block/qcow2.h          |  1 -
>  block/vdi.c            | 11 ++++++-----
>  block/vvfat.c          | 10 ++++++----
>  include/block/block.h  |  4 ----
>  7 files changed, 14 insertions(+), 53 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1556562150.git.berto@igalia.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/cover.1556562150.git.berto@igalia.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1556562150.git.berto@igalia.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/cover.1556562150.git.berto@igalia.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1556562150.git.berto@igalia.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/cover.1556562150.git.berto@igalia.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write()

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1556562150.git.berto@igalia.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/cover.1556562150.git.berto@igalia.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com