[Qemu-devel] [PATCH v2 0/3] block: block driver callbacks fixes

[Qemu-devel] [PATCH v2 0/3] block: block driver callbacks fixes

[Manos Pitsidianakis]

This series makes implementing some of the bdrv_* callbacks easier for block
drivers by passing requests to bs->file if bs->drv doesn't implement it instead
of failing, and adding default bdrv_co_get_block_status() implementations.

This is based against Kevin Wolf's block branch, commit
0dd2ec6bc46bd93ea0b9df602962883417f31400

v2:
  do not pass to bs->file if bs->drv is NULL
  move bs->file check outside of bdrv_inc_in_flight() area in bdrv_co_ioctl()
  new patch: remove duplicate code from block/raw-format.c

Manos Pitsidianakis (3):
  block: pass bdrv_* methods to bs->file by default
  block: use defaults of bdrv_* callbacks in raw
  block: add default implementations for bdrv_co_get_block_status()

 block.c                   | 27 +++++++++++++++++++++++++--
 block/blkdebug.c          | 12 +-----------
 block/commit.c            | 12 +-----------
 block/io.c                | 29 +++++++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 block/raw-format.c        | 32 +-------------------------------
 include/block/block_int.h | 16 ++++++++++++++++
 7 files changed, 74 insertions(+), 66 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Manos Pitsidianakis]

The following functions fail if bs->drv does not implement them:

bdrv_probe_blocksizes
bdrv_probe_geometry
bdrv_truncate
bdrv_has_zero_init
bdrv_get_info
bdrv_media_changed
bdrv_eject
bdrv_lock_medium
bdrv_co_ioctl

Instead, the call should be passed to bs->file if it exists, to allow
filter drivers to support those methods without implementing them.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c    | 27 +++++++++++++++++++++++++--
 block/io.c |  5 +++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 69439628..9e8d34ad 100644
--- a/block.c
+++ b/block.c
@@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 
     if (drv && drv->bdrv_probe_blocksizes) {
         return drv->bdrv_probe_blocksizes(bs, bsz);
+    } else if (drv && bs->file && bs->file->bs) {
+        return bdrv_probe_blocksizes(bs->file->bs, bsz);
     }
 
     return -ENOTSUP;
@@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 
     if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
+    } else if (drv && bs->file && bs->file->bs) {
+        return bdrv_probe_geometry(bs->file->bs, geo);
     }
 
     return -ENOTSUP;
@@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
 
     assert(child->perm & BLK_PERM_RESIZE);
 
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
         error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
     if (!drv->bdrv_truncate) {
+        if (bs->file && bs->file->bs) {
+            return bdrv_truncate(bs->file, offset, errp);
+        }
         error_setg(errp, "Image format driver does not support resize");
         return -ENOTSUP;
     }
@@ -3778,6 +3786,9 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     if (bs->drv->bdrv_has_zero_init) {
         return bs->drv->bdrv_has_zero_init(bs);
     }
+    if (bs->file && bs->file->bs) {
+        return bdrv_has_zero_init(bs->file->bs);
+    }
 
     /* safe default */
     return 0;
@@ -3832,10 +3843,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
+    if (!drv) {
         return -ENOMEDIUM;
-    if (!drv->bdrv_get_info)
+    }
+    if (!drv->bdrv_get_info) {
+        if (bs->file && bs->file->bs) {
+            return bdrv_get_info(bs->file->bs, bdi);
+        }
         return -ENOTSUP;
+    }
     memset(bdi, 0, sizeof(*bdi));
     return drv->bdrv_get_info(bs, bdi);
 }
@@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs)
 
     if (drv && drv->bdrv_media_changed) {
         return drv->bdrv_media_changed(bs);
+    } else if (drv && bs->file && bs->file->bs) {
+        bdrv_media_changed(bs->file->bs);
     }
     return -ENOTSUP;
 }
@@ -4218,6 +4237,8 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag)
 
     if (drv && drv->bdrv_eject) {
         drv->bdrv_eject(bs, eject_flag);
+    } else if (drv && bs->file && bs->file->bs) {
+        bdrv_eject(bs->file->bs, eject_flag);
     }
 }
 
@@ -4233,6 +4254,8 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
 
     if (drv && drv->bdrv_lock_medium) {
         drv->bdrv_lock_medium(bs, locked);
+    } else if (drv && bs->file && bs->file->bs) {
+        bdrv_lock_medium(bs->file->bs, locked);
     }
 }
 
diff --git a/block/io.c b/block/io.c
index c72d7015..a1aee01d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2401,6 +2401,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
     };
     BlockAIOCB *acb;
 
+    if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl &&
+        bs->file && bs->file->bs) {
+        return bdrv_co_ioctl(bs->file->bs, req, buf);
+    }
+
     bdrv_inc_in_flight(bs);
     if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
         co.ret = -ENOTSUP;
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/3] block: use defaults of bdrv_* callbacks in raw

[Manos Pitsidianakis]

Now that passing the call to bs->file is the default for some bdrv_*
callbacks, remove the duplicate implementations in block/raw-format.c

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/raw-format.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index 1ea8c2d7..ff9b4359 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -312,11 +312,6 @@ static int64_t raw_getlength(BlockDriverState *bs)
     return s->size;
 }
 
-static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
-{
-    return bdrv_get_info(bs->file->bs, bdi);
-}
-
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     if (bs->probed) {
@@ -346,21 +341,6 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     return bdrv_truncate(bs->file, offset, errp);
 }
 
-static int raw_media_changed(BlockDriverState *bs)
-{
-    return bdrv_media_changed(bs->file->bs);
-}
-
-static void raw_eject(BlockDriverState *bs, bool eject_flag)
-{
-    bdrv_eject(bs->file->bs, eject_flag);
-}
-
-static void raw_lock_medium(BlockDriverState *bs, bool locked)
-{
-    bdrv_lock_medium(bs->file->bs, locked);
-}
-
 static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
     BDRVRawState *s = bs->opaque;
@@ -370,11 +350,6 @@ static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
     return bdrv_co_ioctl(bs->file->bs, req, buf);
 }
 
-static int raw_has_zero_init(BlockDriverState *bs)
-{
-    return bdrv_has_zero_init(bs->file->bs);
-}
-
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     return bdrv_create_file(filename, opts, errp);
@@ -479,16 +454,11 @@ BlockDriver bdrv_raw = {
     .bdrv_truncate        = &raw_truncate,
     .bdrv_getlength       = &raw_getlength,
     .has_variable_length  = true,
-    .bdrv_get_info        = &raw_get_info,
     .bdrv_refresh_limits  = &raw_refresh_limits,
     .bdrv_probe_blocksizes = &raw_probe_blocksizes,
     .bdrv_probe_geometry  = &raw_probe_geometry,
-    .bdrv_media_changed   = &raw_media_changed,
-    .bdrv_eject           = &raw_eject,
-    .bdrv_lock_medium     = &raw_lock_medium,
     .bdrv_co_ioctl        = &raw_co_ioctl,
-    .create_opts          = &raw_create_opts,
-    .bdrv_has_zero_init   = &raw_has_zero_init
+    .create_opts          = &raw_create_opts
 };
 
 static void bdrv_raw_init(void)
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Manos Pitsidianakis]

bdrv_co_get_block_status_from_file() and
bdrv_co_get_block_status_from_backing() set *file to bs->file and
bs->backing respectively, so that bdrv_co_get_block_status() can recurse
to them. Future block drivers won't have to duplicate code to implement
this.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/blkdebug.c          | 12 +-----------
 block/commit.c            | 12 +-----------
 block/io.c                | 24 ++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 include/block/block_int.h | 16 ++++++++++++++++
 5 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b25856c4..f1539db6 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -641,16 +641,6 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
     return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }
 
-static int64_t coroutine_fn blkdebug_co_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->file->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-        (sector_num << BDRV_SECTOR_BITS);
-}
-
 static void blkdebug_close(BlockDriverState *bs)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -925,7 +915,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_co_flush_to_disk  = blkdebug_co_flush,
     .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkdebug_co_pdiscard,
-    .bdrv_co_get_block_status = blkdebug_co_get_block_status,
+    .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
 
     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
diff --git a/block/commit.c b/block/commit.c
index 524bd549..5b04f832 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -247,16 +247,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
     return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
-static int64_t coroutine_fn bdrv_commit_top_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-           (sector_num << BDRV_SECTOR_BITS);
-}
-
 static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
 {
     bdrv_refresh_filename(bs->backing->bs);
@@ -282,7 +272,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
 static BlockDriver bdrv_commit_top = {
     .format_name                = "commit_top",
     .bdrv_co_preadv             = bdrv_commit_top_preadv,
-    .bdrv_co_get_block_status   = bdrv_commit_top_get_block_status,
+    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
     .bdrv_close                 = bdrv_commit_top_close,
     .bdrv_child_perm            = bdrv_commit_top_child_perm,
diff --git a/block/io.c b/block/io.c
index a1aee01d..2487cc03 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1706,6 +1706,30 @@ typedef struct BdrvCoGetBlockStatusData {
     bool done;
 } BdrvCoGetBlockStatusData;
 
+int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file)
+{
+    assert(bs->file && bs->file->bs);
+    *pnum = nb_sectors;
+    *file = bs->file->bs;
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+           (sector_num << BDRV_SECTOR_BITS);
+}
+
+int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file)
+{
+    assert(bs->backing && bs->backing->bs);
+    *pnum = nb_sectors;
+    *file = bs->backing->bs;
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+           (sector_num << BDRV_SECTOR_BITS);
+}
+
 /*
  * Returns the allocation status of the specified sectors.
  * Drivers not implementing the functionality are assumed to not support
diff --git a/block/mirror.c b/block/mirror.c
index 61a862dc..e8bf5f40 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1052,16 +1052,6 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
     return bdrv_co_flush(bs->backing->bs);
 }
 
-static int64_t coroutine_fn bdrv_mirror_top_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-           (sector_num << BDRV_SECTOR_BITS);
-}
-
 static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
@@ -1108,7 +1098,7 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
-    .bdrv_co_get_block_status   = bdrv_mirror_top_get_block_status,
+    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_close                 = bdrv_mirror_top_close,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa6021..a42c2a6f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -945,6 +945,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared);
 
+/*
+ * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * their file
+ * */
+int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file);
+/*
+ * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * their backing file
+ * */
+int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file);
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
 void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
 bool blk_dev_has_removable_media(BlockBackend *blk);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Eric Blake]

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

On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
> The following functions fail if bs->drv does not implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> bdrv_media_changed
> bdrv_eject
> bdrv_lock_medium
> bdrv_co_ioctl
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c    | 27 +++++++++++++++++++++++++--
>  block/io.c |  5 +++++
>  2 files changed, 30 insertions(+), 2 deletions(-)

Did you check whether any other existing drivers can be cleaned up to
rely on the defaults?  Or is it just the raw driver that you cleaned in 2/3?

At any rate, this makes sense to me.  However,

> @@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs)
>  
>      if (drv && drv->bdrv_media_changed) {
>          return drv->bdrv_media_changed(bs);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        bdrv_media_changed(bs->file->bs);
>      }
>      return -ENOTSUP;

This one returns -ENOTSUP unconditionally after recursing; looks like
you omitted 'return'.

If that's the only fix you make for v3, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 2/3] block: use defaults of bdrv_* callbacks in raw

[Eric Blake]

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

On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
> Now that passing the call to bs->file is the default for some bdrv_*
> callbacks, remove the duplicate implementations in block/raw-format.c
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block/raw-format.c | 32 +-------------------------------
>  1 file changed, 1 insertion(+), 31 deletions(-)
> 

> @@ -479,16 +454,11 @@ BlockDriver bdrv_raw = {
>      .bdrv_truncate        = &raw_truncate,
>      .bdrv_getlength       = &raw_getlength,
>      .has_variable_length  = true,
> -    .bdrv_get_info        = &raw_get_info,
>      .bdrv_refresh_limits  = &raw_refresh_limits,
>      .bdrv_probe_blocksizes = &raw_probe_blocksizes,
>      .bdrv_probe_geometry  = &raw_probe_geometry,
> -    .bdrv_media_changed   = &raw_media_changed,
> -    .bdrv_eject           = &raw_eject,
> -    .bdrv_lock_medium     = &raw_lock_medium,
>      .bdrv_co_ioctl        = &raw_co_ioctl,
> -    .create_opts          = &raw_create_opts,
> -    .bdrv_has_zero_init   = &raw_has_zero_init
> +    .create_opts          = &raw_create_opts

Please use a trailing comma here. It causes less churn in future patches
if you don't have to have a -/+ line pair just re-adding the comma.
(Yes, we didn't have a trailing comma, but we should have had one; your
patch is now the chance to make it so).

With that fixed, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Eric Blake]

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

On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
> bdrv_co_get_block_status_from_file() and
> bdrv_co_get_block_status_from_backing() set *file to bs->file and
> bs->backing respectively, so that bdrv_co_get_block_status() can recurse
> to them. Future block drivers won't have to duplicate code to implement
> this.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block/blkdebug.c          | 12 +-----------
>  block/commit.c            | 12 +-----------
>  block/io.c                | 24 ++++++++++++++++++++++++
>  block/mirror.c            | 12 +-----------
>  include/block/block_int.h | 16 ++++++++++++++++
>  5 files changed, 43 insertions(+), 33 deletions(-)

My [still-needs-rebasing] part 3 series converting
bdrv_co_get_block_status to byte-based will be impacted by this, but I'd
rather yours go in first and I can deal with the rebase fallout.


> +++ b/include/block/block_int.h
> @@ -945,6 +945,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>                                 uint64_t perm, uint64_t shared,
>                                 uint64_t *nperm, uint64_t *nshared);
>  
> +/*
> + * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * their file

I would have ended with '.'

> + * */

Looks odd.  Just use ' */', not ' * */'

> +int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int *pnum,
> +                                                     BlockDriverState **file);

Indentation is unusual (not necessarily bad, since you are up against
80-column limits).

> +/*
> + * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * their backing file
> + * */
> +int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int *pnum,
> +                                                     BlockDriverState **file);

Ditto.

Making changes according to what I mentioned is minor, so you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Eric Blake]

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

On 07/03/2017 11:12 AM, Eric Blake wrote:
> On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
>> bdrv_co_get_block_status_from_file() and
>> bdrv_co_get_block_status_from_backing() set *file to bs->file and
>> bs->backing respectively, so that bdrv_co_get_block_status() can recurse
>> to them. Future block drivers won't have to duplicate code to implement
>> this.
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>>  block/blkdebug.c          | 12 +-----------
>>  block/commit.c            | 12 +-----------
>>  block/io.c                | 24 ++++++++++++++++++++++++
>>  block/mirror.c            | 12 +-----------
>>  include/block/block_int.h | 16 ++++++++++++++++
>>  5 files changed, 43 insertions(+), 33 deletions(-)
> 
> My [still-needs-rebasing] part 3 series converting
> bdrv_co_get_block_status to byte-based will be impacted by this, but I'd
> rather yours go in first and I can deal with the rebase fallout.

In fact, my rebase fallout is to completely delete
bdrv_co_get_block_status_from_file().  Why? Because blkdebug is the only
client, but I want to enhance blkdebug to add in-place assert()ions that
the values passed by the block layer are properly aligned to
bs->bl.request_alignment (matching what blkdebug does for read and write
- after all, if a device cannot access smaller than a given alignment
for a read, it should not be able to report different statuses half-way
through that granularity).  However, in-place assertions are no longer
generic, so we no longer have a client of a generic helper function
referring to bs->file.

Which means you are REALLY left only with commit and mirror as the two
functions that have a common implementation of pointing back to backing.

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


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

Re: [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Manos Pitsidianakis]

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

On Mon, Jul 03, 2017 at 12:51:45PM -0500, Eric Blake wrote:
>On 07/03/2017 11:12 AM, Eric Blake wrote:
>> On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
>>> bdrv_co_get_block_status_from_file() and
>>> bdrv_co_get_block_status_from_backing() set *file to bs->file and
>>> bs->backing respectively, so that bdrv_co_get_block_status() can recurse
>>> to them. Future block drivers won't have to duplicate code to implement
>>> this.
>>>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>>> ---
>>>  block/blkdebug.c          | 12 +-----------
>>>  block/commit.c            | 12 +-----------
>>>  block/io.c                | 24 ++++++++++++++++++++++++
>>>  block/mirror.c            | 12 +-----------
>>>  include/block/block_int.h | 16 ++++++++++++++++
>>>  5 files changed, 43 insertions(+), 33 deletions(-)
>>
>> My [still-needs-rebasing] part 3 series converting
>> bdrv_co_get_block_status to byte-based will be impacted by this, but I'd
>> rather yours go in first and I can deal with the rebase fallout.
>
>In fact, my rebase fallout is to completely delete
>bdrv_co_get_block_status_from_file().  Why? Because blkdebug is the only
>client, but I want to enhance blkdebug to add in-place assert()ions that
>the values passed by the block layer are properly aligned to
>bs->bl.request_alignment (matching what blkdebug does for read and write
>- after all, if a device cannot access smaller than a given alignment
>for a read, it should not be able to report different statuses half-way
>through that granularity).  However, in-place assertions are no longer
>generic, so we no longer have a client of a generic helper function
>referring to bs->file.
>
>Which means you are REALLY left only with commit and mirror as the two
>functions that have a common implementation of pointing back to 
>backing.


Hello Eric, thanks for taking the time to review my patches.

The throttle driver I'm working on passes bdrv_co_get_block_status() to 
bs->file. If there is a problem with an unused default function (it's 
not static so will compile, but it might not be up to standard), you can 
just remove it and I will reintroduce it when it's needed. CC me on 
those patches when you send them if you can.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Eric Blake]

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

On 07/03/2017 01:24 PM, Manos Pitsidianakis wrote:

>> In fact, my rebase fallout is to completely delete
>> bdrv_co_get_block_status_from_file().  Why? Because blkdebug is the only
>> client, but I want to enhance blkdebug to add in-place assert()ions that
>> the values passed by the block layer are properly aligned to
>> bs->bl.request_alignment (matching what blkdebug does for read and write
>> - after all, if a device cannot access smaller than a given alignment
>> for a read, it should not be able to report different statuses half-way
>> through that granularity).  However, in-place assertions are no longer
>> generic, so we no longer have a client of a generic helper function
>> referring to bs->file.
>>
>> Which means you are REALLY left only with commit and mirror as the two
>> functions that have a common implementation of pointing back to backing.
> 
> 
> Hello Eric, thanks for taking the time to review my patches.
> 
> The throttle driver I'm working on passes bdrv_co_get_block_status() to
> bs->file. If there is a problem with an unused default function (it's
> not static so will compile, but it might not be up to standard), you can
> just remove it and I will reintroduce it when it's needed. CC me on
> those patches when you send them if you can.

Sure. The other thing I can do is have:

blkdebug_version() {
   assert(...);
   return common version;
}

so that the common version is still there for use by the throttling
code.  I'm adjusting my rebase accordingly, now that I know you have
another intended use in mind.

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


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

Re: [Qemu-devel] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Eric Blake]

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

On 07/03/2017 10:56 AM, Eric Blake wrote:
> On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
>> The following functions fail if bs->drv does not implement them:

One other suggestion: most patches in the tree use 'topic: Capital',
while yours used 'topic: lowercase'.  It's probably worth posting a v3
that addresses all of the nits I've pointed out, including tweaking the
commit messages.

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


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

Re: [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Eric Blake]

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

On 07/03/2017 01:31 PM, Eric Blake wrote:
>> The throttle driver I'm working on passes bdrv_co_get_block_status() to
>> bs->file. If there is a problem with an unused default function (it's
>> not static so will compile, but it might not be up to standard), you can
>> just remove it and I will reintroduce it when it's needed. CC me on
>> those patches when you send them if you can.
> 
> Sure.

Now posted:
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00427.html

> The other thing I can do is have:
> 
> blkdebug_version() {
>    assert(...);
>    return common version;
> }

That's the approach I went with in 14/15 of that series.

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Stefan Hajnoczi]

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

On Mon, Jul 03, 2017 at 01:40:57PM -0500, Eric Blake wrote:
> On 07/03/2017 10:56 AM, Eric Blake wrote:
> > On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
> >> The following functions fail if bs->drv does not implement them:
> 
> One other suggestion: most patches in the tree use 'topic: Capital',
> while yours used 'topic: lowercase'.  It's probably worth posting a v3
> that addresses all of the nits I've pointed out, including tweaking the
> commit messages.

Plenty of regular contributors use "topic: lowercase" including Michael
Tsirkin, Mark Cave-Ayland, Peter Xu, and myself (just from looking at
the first page of git log --oneline on qemu.git/master).

If we want to enforce a consistent style in the future then it should be
discussed separately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Stefan Hajnoczi]

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

On Thu, Jun 29, 2017 at 09:43:18PM +0300, Manos Pitsidianakis wrote:
> diff --git a/block.c b/block.c
> index 69439628..9e8d34ad 100644
> --- a/block.c
> +++ b/block.c
> @@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>  
>      if (drv && drv->bdrv_probe_blocksizes) {
>          return drv->bdrv_probe_blocksizes(bs, bsz);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        return bdrv_probe_blocksizes(bs->file->bs, bsz);
>      }
>  
>      return -ENOTSUP;

Currently only raw-format.c and file-posix.c implement
bdrv_probe_blocksizes().  qcow2 will start reporting bs->file's
blocksizes after this patch.

This can lead to a change in blocksizes when live migrating from an old
QEMU to a new QEMU.  Guest operating systems and applications can be
confused if the device suddenly changes beneath them.

On the other hand, it's already possible to hit that today by migrating
from a raw image to a qcow2 image.

I think this change makes sense - we should propagate blocksizes through
the graph - but it may introduce an incompatibility.

Kevin: Do you think this is safe?

> @@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
>  
>      assert(child->perm & BLK_PERM_RESIZE);
>  
> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
>      if (!drv) {
>          error_setg(errp, "No medium inserted");
>          return -ENOMEDIUM;
>      }
>      if (!drv->bdrv_truncate) {
> +        if (bs->file && bs->file->bs) {
> +            return bdrv_truncate(bs->file, offset, errp);
> +        }
>          error_setg(errp, "Image format driver does not support resize");
>          return -ENOTSUP;
>      }

This is not safe because existing image formats (e.g. vmdk, dmg) do not
implement .bdrv_truncate().  If we begin truncating the underlying host
file ("foo.vmdk") the disk image will be corrupted.

It is only safe to forward .bdrv_truncate() in filter drivers.  I
suggest providing a default implementation instead:

int bdrv_truncate_file(...);

Then filters can do:

  .bdrv_truncate = bdrv_truncate_file,

> diff --git a/block/io.c b/block/io.c
> index c72d7015..a1aee01d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2401,6 +2401,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
>      };
>      BlockAIOCB *acb;
>  
> +    if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl &&
> +        bs->file && bs->file->bs) {
> +        return bdrv_co_ioctl(bs->file->bs, req, buf);
> +    }
> +
>      bdrv_inc_in_flight(bs);
>      if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
>          co.ret = -ENOTSUP;

This operation cannot be allowed by default for the same reason as
.bdrv_truncate().  It could change the host file in ways that the format
driver can't handle.  A separate function is needed here:
bdrv_co_ioctl_file().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 2/3] block: use defaults of bdrv_* callbacks in raw

[Stefan Hajnoczi]

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

On Thu, Jun 29, 2017 at 09:43:19PM +0300, Manos Pitsidianakis wrote:
> Now that passing the call to bs->file is the default for some bdrv_*
> callbacks, remove the duplicate implementations in block/raw-format.c
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block/raw-format.c | 32 +-------------------------------
>  1 file changed, 1 insertion(+), 31 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]