[PATCH v8 0/7] coroutines: generate wrapper code

[PATCH v8 0/7] coroutines: generate wrapper code

[Vladimir Sementsov-Ogievskiy]

Hi all!

The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.

Benefits:
 - no code duplication
 - less indirection

v8:
04: - rebase on meson build
        - script interface is changed to satisfy meson custom_target
    - rename script s/coroutine-wrapper.py/block-coroutine-wrapper.py/
    - add docs/devel/block-coroutine-wrapper.rst

Vladimir Sementsov-Ogievskiy (7):
  block: return error-code from bdrv_invalidate_cache
  block/io: refactor coroutine wrappers
  block: declare some coroutine functions in block/coroutines.h
  scripts: add block-coroutine-wrapper.py
  block: generate coroutine-wrapper code
  block: drop bdrv_prwv
  block/io: refactor save/load vmstate

 docs/devel/block-coroutine-wrapper.rst |  54 ++++
 block/block-gen.h                      |  49 ++++
 block/coroutines.h                     |  65 +++++
 include/block/block.h                  |  34 ++-
 block.c                                |  97 ++-----
 block/io.c                             | 336 ++++---------------------
 tests/test-bdrv-drain.c                |   2 +-
 block/meson.build                      |   8 +
 scripts/block-coroutine-wrapper.py     | 187 ++++++++++++++
 9 files changed, 451 insertions(+), 381 deletions(-)
 create mode 100644 docs/devel/block-coroutine-wrapper.rst
 create mode 100644 block/block-gen.h
 create mode 100644 block/coroutines.h
 create mode 100755 scripts/block-coroutine-wrapper.py

-- 
2.21.3

[PATCH v8 1/7] block: return error-code from bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

This is the only coroutine wrapper from block.c and block/io.c which
doesn't return a value, so let's convert it to the common behavior, to
simplify moving to generated coroutine wrappers in a further commit.

Also, bdrv_invalidate_cache is a void function, returning error only
through **errp parameter, which is considered to be bad practice, as
it forces callers to define and propagate local_err variable, so
conversion is good anyway.

This patch leaves the conversion of .bdrv_co_invalidate_cache() driver
callbacks and bdrv_invalidate_cache_all() for another day.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h |  2 +-
 block.c               | 32 ++++++++++++++++++--------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 6e36154061..8aef849a75 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -460,7 +460,7 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb);
 int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
 
 /* Invalidate any cached metadata used by image formats */
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
+int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
 void bdrv_invalidate_cache_all(Error **errp);
 int bdrv_inactivate_all(void);
 
diff --git a/block.c b/block.c
index 2ba76b2c36..ccfe1d851b 100644
--- a/block.c
+++ b/block.c
@@ -5649,8 +5649,8 @@ void bdrv_init_with_whitelist(void)
     bdrv_init();
 }
 
-static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
-                                                  Error **errp)
+static int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
+                                                 Error **errp)
 {
     BdrvChild *child, *parent;
     uint64_t perm, shared_perm;
@@ -5659,14 +5659,14 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
     BdrvDirtyBitmap *bm;
 
     if (!bs->drv)  {
-        return;
+        return -ENOMEDIUM;
     }
 
     QLIST_FOREACH(child, &bs->children, next) {
         bdrv_co_invalidate_cache(child->bs, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            return;
+            return -EINVAL;
         }
     }
 
@@ -5689,7 +5689,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
         ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
         if (ret < 0) {
             bs->open_flags |= BDRV_O_INACTIVE;
-            return;
+            return ret;
         }
         bdrv_set_perm(bs, perm, shared_perm);
 
@@ -5698,7 +5698,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
             if (local_err) {
                 bs->open_flags |= BDRV_O_INACTIVE;
                 error_propagate(errp, local_err);
-                return;
+                return -EINVAL;
             }
         }
 
@@ -5710,7 +5710,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
         if (ret < 0) {
             bs->open_flags |= BDRV_O_INACTIVE;
             error_setg_errno(errp, -ret, "Could not refresh total sector count");
-            return;
+            return ret;
         }
     }
 
@@ -5720,27 +5720,30 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
             if (local_err) {
                 bs->open_flags |= BDRV_O_INACTIVE;
                 error_propagate(errp, local_err);
-                return;
+                return -EINVAL;
             }
         }
     }
+
+    return 0;
 }
 
 typedef struct InvalidateCacheCo {
     BlockDriverState *bs;
     Error **errp;
     bool done;
+    int ret;
 } InvalidateCacheCo;
 
 static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
 {
     InvalidateCacheCo *ico = opaque;
-    bdrv_co_invalidate_cache(ico->bs, ico->errp);
+    ico->ret = bdrv_co_invalidate_cache(ico->bs, ico->errp);
     ico->done = true;
     aio_wait_kick();
 }
 
-void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
+int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     Coroutine *co;
     InvalidateCacheCo ico = {
@@ -5757,22 +5760,23 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         bdrv_coroutine_enter(bs, co);
         BDRV_POLL_WHILE(bs, !ico.done);
     }
+
+    return ico.ret;
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
 {
     BlockDriverState *bs;
-    Error *local_err = NULL;
     BdrvNextIterator it;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
+        int ret;
 
         aio_context_acquire(aio_context);
-        bdrv_invalidate_cache(bs, &local_err);
+        ret = bdrv_invalidate_cache(bs, errp);
         aio_context_release(aio_context);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (ret < 0) {
             bdrv_next_cleanup(&it);
             return;
         }
-- 
2.21.3

Re: [PATCH v8 1/7] block: return error-code from bdrv_invalidate_cache

[Philippe Mathieu-Daudé]

On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> This is the only coroutine wrapper from block.c and block/io.c which
> doesn't return a value, so let's convert it to the common behavior, to
> simplify moving to generated coroutine wrappers in a further commit.
> 
> Also, bdrv_invalidate_cache is a void function, returning error only
> through **errp parameter, which is considered to be bad practice, as
> it forces callers to define and propagate local_err variable, so
> conversion is good anyway.
> 
> This patch leaves the conversion of .bdrv_co_invalidate_cache() driver
> callbacks and bdrv_invalidate_cache_all() for another day.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block.h |  2 +-
>  block.c               | 32 ++++++++++++++++++--------------
>  2 files changed, 19 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v8 1/7] block: return error-code from bdrv_invalidate_cache

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> This is the only coroutine wrapper from block.c and block/io.c which
> doesn't return a value, so let's convert it to the common behavior, to
> simplify moving to generated coroutine wrappers in a further commit.
> 
> Also, bdrv_invalidate_cache is a void function, returning error only
> through **errp parameter, which is considered to be bad practice, as
> it forces callers to define and propagate local_err variable, so
> conversion is good anyway.
> 
> This patch leaves the conversion of .bdrv_co_invalidate_cache() driver
> callbacks and bdrv_invalidate_cache_all() for another day.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block.h |  2 +-
>  block.c               | 32 ++++++++++++++++++--------------
>  2 files changed, 19 insertions(+), 15 deletions(-)

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

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

[PATCH v8 2/7] block/io: refactor coroutine wrappers

[Vladimir Sementsov-Ogievskiy]

Most of our coroutine wrappers already follow this convention:

We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
the core function, and a wrapper 'bdrv_<something>(<same argument
list>)' which does parameters packing and call bdrv_run_co().

The only outsiders are the bdrv_prwv_co and
bdrv_common_block_status_above wrappers. Let's refactor them to behave
as the others, it simplifies further conversion of coroutine wrappers.

This patch adds indirection layer, but it will be compensated by
further commit, which will drop bdrv_co_prwv together with is_write
logic, to keep read and write path separate.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 60 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/block/io.c b/block/io.c
index ad3a51ed53..2e2c89ce31 100644
--- a/block/io.c
+++ b/block/io.c
@@ -933,27 +933,31 @@ typedef struct RwCo {
     BdrvRequestFlags flags;
 } RwCo;
 
+static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
+                                     QEMUIOVector *qiov, bool is_write,
+                                     BdrvRequestFlags flags)
+{
+    if (is_write) {
+        return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
+    } else {
+        return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
+    }
+}
+
 static int coroutine_fn bdrv_rw_co_entry(void *opaque)
 {
     RwCo *rwco = opaque;
 
-    if (!rwco->is_write) {
-        return bdrv_co_preadv(rwco->child, rwco->offset,
-                              rwco->qiov->size, rwco->qiov,
-                              rwco->flags);
-    } else {
-        return bdrv_co_pwritev(rwco->child, rwco->offset,
-                               rwco->qiov->size, rwco->qiov,
-                               rwco->flags);
-    }
+    return bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
+                        rwco->is_write, rwco->flags);
 }
 
 /*
  * Process a vectored synchronous request using coroutines
  */
-static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
-                        QEMUIOVector *qiov, bool is_write,
-                        BdrvRequestFlags flags)
+static int bdrv_prwv(BdrvChild *child, int64_t offset,
+                     QEMUIOVector *qiov, bool is_write,
+                     BdrvRequestFlags flags)
 {
     RwCo rwco = {
         .child = child,
@@ -971,8 +975,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
 {
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
 
-    return bdrv_prwv_co(child, offset, &qiov, true,
-                        BDRV_REQ_ZERO_WRITE | flags);
+    return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
@@ -1021,7 +1024,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(child, offset, qiov, false, 0);
+    ret = bdrv_prwv(child, offset, qiov, false, 0);
     if (ret < 0) {
         return ret;
     }
@@ -1045,7 +1048,7 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 {
     int ret;
 
-    ret = bdrv_prwv_co(child, offset, qiov, true, 0);
+    ret = bdrv_prwv(child, offset, qiov, true, 0);
     if (ret < 0) {
         return ret;
     }
@@ -2465,14 +2468,15 @@ early_out:
     return ret;
 }
 
-static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
-                                                   BlockDriverState *base,
-                                                   bool want_zero,
-                                                   int64_t offset,
-                                                   int64_t bytes,
-                                                   int64_t *pnum,
-                                                   int64_t *map,
-                                                   BlockDriverState **file)
+static int coroutine_fn
+bdrv_co_common_block_status_above(BlockDriverState *bs,
+                                  BlockDriverState *base,
+                                  bool want_zero,
+                                  int64_t offset,
+                                  int64_t bytes,
+                                  int64_t *pnum,
+                                  int64_t *map,
+                                  BlockDriverState **file)
 {
     BlockDriverState *p;
     int ret = 0;
@@ -2510,10 +2514,10 @@ static int coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
 {
     BdrvCoBlockStatusData *data = opaque;
 
-    return bdrv_co_block_status_above(data->bs, data->base,
-                                      data->want_zero,
-                                      data->offset, data->bytes,
-                                      data->pnum, data->map, data->file);
+    return bdrv_co_common_block_status_above(data->bs, data->base,
+                                             data->want_zero,
+                                             data->offset, data->bytes,
+                                             data->pnum, data->map, data->file);
 }
 
 /*
-- 
2.21.3

Re: [PATCH v8 2/7] block/io: refactor coroutine wrappers

[Eric Blake]

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Most of our coroutine wrappers already follow this convention:
> 
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
> the core function, and a wrapper 'bdrv_<something>(<same argument
> list>)' which does parameters packing and call bdrv_run_co().
> 
> The only outsiders are the bdrv_prwv_co and
> bdrv_common_block_status_above wrappers. Let's refactor them to behave
> as the others, it simplifies further conversion of coroutine wrappers.
> 
> This patch adds indirection layer, but it will be compensated by

adds an

> further commit, which will drop bdrv_co_prwv together with is_write
> logic, to keep read and write path separate.

keep the

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

R-b stands.

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

Re: [PATCH v8 2/7] block/io: refactor coroutine wrappers

[Philippe Mathieu-Daudé]

On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> Most of our coroutine wrappers already follow this convention:
> 
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
> the core function, and a wrapper 'bdrv_<something>(<same argument
> list>)' which does parameters packing and call bdrv_run_co().
> 
> The only outsiders are the bdrv_prwv_co and
> bdrv_common_block_status_above wrappers. Let's refactor them to behave
> as the others, it simplifies further conversion of coroutine wrappers.
> 
> This patch adds indirection layer, but it will be compensated by
> further commit, which will drop bdrv_co_prwv together with is_write
> logic, to keep read and write path separate.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/io.c | 60 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 28 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v8 2/7] block/io: refactor coroutine wrappers

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Most of our coroutine wrappers already follow this convention:
> 
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
> the core function, and a wrapper 'bdrv_<something>(<same argument
> list>)' which does parameters packing and call bdrv_run_co().
> 
> The only outsiders are the bdrv_prwv_co and
> bdrv_common_block_status_above wrappers. Let's refactor them to behave
> as the others, it simplifies further conversion of coroutine wrappers.
> 
> This patch adds indirection layer, but it will be compensated by
> further commit, which will drop bdrv_co_prwv together with is_write
> logic, to keep read and write path separate.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/io.c | 60 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 28 deletions(-)

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

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

[PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h

[Vladimir Sementsov-Ogievskiy]

We are going to keep coroutine-wrappers code (structure-packing
parameters, BDRV_POLL wrapper functions) in separate auto-generated
files. So, we'll need a header with declaration of original _co_
functions, for those which are static now. As well, we'll need
declarations for wrapper functions. Do these declarations now, as a
preparation step.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/coroutines.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 block.c            |  8 +++---
 block/io.c         | 34 +++++++++++------------
 3 files changed, 88 insertions(+), 21 deletions(-)
 create mode 100644 block/coroutines.h

diff --git a/block/coroutines.h b/block/coroutines.h
new file mode 100644
index 0000000000..9ce1730a09
--- /dev/null
+++ b/block/coroutines.h
@@ -0,0 +1,67 @@
+/*
+ * Block layer I/O functions
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_COROUTINES_INT_H
+#define BLOCK_COROUTINES_INT_H
+
+#include "block/block_int.h"
+
+int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+                               BdrvCheckResult *res, BdrvCheckMode fix);
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
+
+int coroutine_fn
+bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
+             bool is_write, BdrvRequestFlags flags);
+int
+bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
+          bool is_write, BdrvRequestFlags flags);
+
+int coroutine_fn
+bdrv_co_common_block_status_above(BlockDriverState *bs,
+                                  BlockDriverState *base,
+                                  bool want_zero,
+                                  int64_t offset,
+                                  int64_t bytes,
+                                  int64_t *pnum,
+                                  int64_t *map,
+                                  BlockDriverState **file);
+int
+bdrv_common_block_status_above(BlockDriverState *bs,
+                               BlockDriverState *base,
+                               bool want_zero,
+                               int64_t offset,
+                               int64_t bytes,
+                               int64_t *pnum,
+                               int64_t *map,
+                               BlockDriverState **file);
+
+int coroutine_fn
+bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+                   bool is_read);
+int
+bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+                bool is_read);
+
+#endif /* BLOCK_COROUTINES_INT_H */
diff --git a/block.c b/block.c
index ccfe1d851b..ec5a8cbd7b 100644
--- a/block.c
+++ b/block.c
@@ -48,6 +48,7 @@
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
 #include "qemu/id.h"
+#include "block/coroutines.h"
 
 #ifdef CONFIG_BSD
 #include <sys/ioctl.h>
@@ -4640,8 +4641,8 @@ static void bdrv_delete(BlockDriverState *bs)
  * free of errors) or -errno when an internal error occurred. The results of the
  * check are stored in res.
  */
-static int coroutine_fn bdrv_co_check(BlockDriverState *bs,
-                                      BdrvCheckResult *res, BdrvCheckMode fix)
+int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+                               BdrvCheckResult *res, BdrvCheckMode fix)
 {
     if (bs->drv == NULL) {
         return -ENOMEDIUM;
@@ -5649,8 +5650,7 @@ void bdrv_init_with_whitelist(void)
     bdrv_init();
 }
 
-static int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
-                                                 Error **errp)
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BdrvChild *child, *parent;
     uint64_t perm, shared_perm;
diff --git a/block/io.c b/block/io.c
index 2e2c89ce31..676c932caf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -29,6 +29,7 @@
 #include "block/blockjob.h"
 #include "block/blockjob_int.h"
 #include "block/block_int.h"
+#include "block/coroutines.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -933,9 +934,9 @@ typedef struct RwCo {
     BdrvRequestFlags flags;
 } RwCo;
 
-static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
-                                     QEMUIOVector *qiov, bool is_write,
-                                     BdrvRequestFlags flags)
+int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
+                              QEMUIOVector *qiov, bool is_write,
+                              BdrvRequestFlags flags)
 {
     if (is_write) {
         return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
@@ -955,9 +956,9 @@ static int coroutine_fn bdrv_rw_co_entry(void *opaque)
 /*
  * Process a vectored synchronous request using coroutines
  */
-static int bdrv_prwv(BdrvChild *child, int64_t offset,
-                     QEMUIOVector *qiov, bool is_write,
-                     BdrvRequestFlags flags)
+int bdrv_prwv(BdrvChild *child, int64_t offset,
+              QEMUIOVector *qiov, bool is_write,
+              BdrvRequestFlags flags)
 {
     RwCo rwco = {
         .child = child,
@@ -2468,7 +2469,7 @@ early_out:
     return ret;
 }
 
-static int coroutine_fn
+int coroutine_fn
 bdrv_co_common_block_status_above(BlockDriverState *bs,
                                   BlockDriverState *base,
                                   bool want_zero,
@@ -2525,12 +2526,12 @@ static int coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
  *
  * See bdrv_co_block_status_above() for details.
  */
-static int bdrv_common_block_status_above(BlockDriverState *bs,
-                                          BlockDriverState *base,
-                                          bool want_zero, int64_t offset,
-                                          int64_t bytes, int64_t *pnum,
-                                          int64_t *map,
-                                          BlockDriverState **file)
+int bdrv_common_block_status_above(BlockDriverState *bs,
+                                   BlockDriverState *base,
+                                   bool want_zero, int64_t offset,
+                                   int64_t bytes, int64_t *pnum,
+                                   int64_t *map,
+                                   BlockDriverState **file)
 {
     BdrvCoBlockStatusData data = {
         .bs = bs,
@@ -2646,7 +2647,7 @@ typedef struct BdrvVmstateCo {
     bool                is_read;
 } BdrvVmstateCo;
 
-static int coroutine_fn
+int coroutine_fn
 bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
                    bool is_read)
 {
@@ -2678,9 +2679,8 @@ static int coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
     return bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
 }
 
-static inline int
-bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-                bool is_read)
+int bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
+                    bool is_read)
 {
     BdrvVmstateCo data = {
         .bs         = bs,
-- 
2.21.3

Re: [PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h

[Eric Blake]

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to keep coroutine-wrappers code (structure-packing
> parameters, BDRV_POLL wrapper functions) in separate auto-generated
> files. So, we'll need a header with declaration of original _co_
> functions, for those which are static now. As well, we'll need
> declarations for wrapper functions. Do these declarations now, as a
> preparation step.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---

> +++ b/block/coroutines.h

> +int coroutine_fn bdrv_co_check(BlockDriverState *bs,
> +                               BdrvCheckResult *res, BdrvCheckMode fix);
> +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
> +
> +int coroutine_fn
> +bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
> +             bool is_write, BdrvRequestFlags flags);

Inconsistent on whether the function name is in column 1 or after the 
return type. But we aren't consistent elsewhre, so I won't bother 
changing it.

R-b still stands

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

Re: [PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h

[Philippe Mathieu-Daudé]

On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to keep coroutine-wrappers code (structure-packing
> parameters, BDRV_POLL wrapper functions) in separate auto-generated
> files. So, we'll need a header with declaration of original _co_
> functions, for those which are static now. As well, we'll need
> declarations for wrapper functions. Do these declarations now, as a
> preparation step.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  block.c            |  8 +++---
>  block/io.c         | 34 +++++++++++------------
>  3 files changed, 88 insertions(+), 21 deletions(-)
>  create mode 100644 block/coroutines.h
> 
> diff --git a/block/coroutines.h b/block/coroutines.h
> new file mode 100644
> index 0000000000..9ce1730a09
> --- /dev/null
> +++ b/block/coroutines.h
> @@ -0,0 +1,67 @@

Maybe also add:

   /* SPDX-License-Identifier: MIT */

> +/*
> + * Block layer I/O functions
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef BLOCK_COROUTINES_INT_H
> +#define BLOCK_COROUTINES_INT_H
> +
> +#include "block/block_int.h"
> +
> +int coroutine_fn bdrv_co_check(BlockDriverState *bs,
> +                               BdrvCheckResult *res, BdrvCheckMode fix);
> +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
> +
> +int coroutine_fn
> +bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
> +             bool is_write, BdrvRequestFlags flags);
> +int
> +bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
> +          bool is_write, BdrvRequestFlags flags);
> +
> +int coroutine_fn
> +bdrv_co_common_block_status_above(BlockDriverState *bs,
> +                                  BlockDriverState *base,
> +                                  bool want_zero,
> +                                  int64_t offset,
> +                                  int64_t bytes,
> +                                  int64_t *pnum,
> +                                  int64_t *map,
> +                                  BlockDriverState **file);
> +int
> +bdrv_common_block_status_above(BlockDriverState *bs,
> +                               BlockDriverState *base,
> +                               bool want_zero,
> +                               int64_t offset,
> +                               int64_t bytes,
> +                               int64_t *pnum,
> +                               int64_t *map,
> +                               BlockDriverState **file);
> +

Prototypes documentation welcomed, but this is rather scarce
in the block APIs, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +int coroutine_fn
> +bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
> +                   bool is_read);
> +int
> +bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
> +                bool is_read);
> +
> +#endif /* BLOCK_COROUTINES_INT_H */

Re: [PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are going to keep coroutine-wrappers code (structure-packing
> parameters, BDRV_POLL wrapper functions) in separate auto-generated
> files. So, we'll need a header with declaration of original _co_
> functions, for those which are static now. As well, we'll need
> declarations for wrapper functions. Do these declarations now, as a
> preparation step.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  block.c            |  8 +++---
>  block/io.c         | 34 +++++++++++------------
>  3 files changed, 88 insertions(+), 21 deletions(-)
>  create mode 100644 block/coroutines.h

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

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

[PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

We have a very frequent pattern of creating coroutine from function
with several arguments:

  - create structure to pack parameters
  - create _entry function to call original function taking parameters
    from struct
  - do different magic to handle completion: set ret to NOT_DONE or
    EINPROGRESS or use separate bool field
  - fill the struct and create coroutine from _entry function and this
    struct as a parameter
  - do coroutine enter and BDRV_POLL_WHILE loop

Let's reduce code duplication by generating coroutine wrappers.

This patch adds scripts/block-coroutine-wrapper.py together with some
friends, which will generate functions with declared prototypes marked
by 'generated_co_wrapper' specifier.

The usage of new code generation is as follows:

    1. define somewhere

        int coroutine_fn bdrv_co_NAME(...) {...}

       function

    2. declare in some header file

        int generated_co_wrapper bdrv_NAME(...);

       function with same list of parameters. (you'll need to include
       "block/generated-co-wrapper.h" to get the specifier)

    3. both declarations should be available through block/coroutines.h
       header.

    4. add header with generated_co_wrapper declaration into
       COROUTINE_HEADERS list in Makefile

Still, no function is now marked, this work is for the following
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/devel/block-coroutine-wrapper.rst |  54 +++++++
 block/block-gen.h                      |  49 +++++++
 include/block/block.h                  |  10 ++
 block/meson.build                      |   8 ++
 scripts/block-coroutine-wrapper.py     | 187 +++++++++++++++++++++++++
 5 files changed, 308 insertions(+)
 create mode 100644 docs/devel/block-coroutine-wrapper.rst
 create mode 100644 block/block-gen.h
 create mode 100755 scripts/block-coroutine-wrapper.py

diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
new file mode 100644
index 0000000000..f7050bbc8f
--- /dev/null
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+=======================
+block-coroutine-wrapper
+=======================
+
+A lot of functions in QEMJ block layer (see ``block/*``) can by called
+only in coroutine context. Such functions are normally marked by
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context, for this we need to start a coroutine, run the
+needed function from it and wait for coroutine finish in
+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function, which needs
+non-coroutine interface, we should define a structure to pack the
+parameters, define a separate function to unpack the parameters and
+call the original function and finally define a new interface function
+with same list of arguments as original one, which will pack the
+parameters into a struct, create a coroutine, run it and wait in
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
+we have a script to generate them.
+
+Usage
+=====
+
+Assume we have defined ``coroutine_fn`` function
+``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
+called ``bdrv_foo(<same args>)``. In this case the script can help. To
+trigger the generation:
+
+1. You need ``bdrv_foo`` declaration somewhere (for example in
+   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
+   like this:
+
+.. code-block:: c
+
+    int generated_co_wrapper bdrv_foor(<some args>);
+
+2. You need to feed this declaration to block-coroutine-wrapper script.
+   For this, add .h (or .c) file with the declaration to
+   ``input: files(...)`` list of ``block_gen_c`` target declaration in
+   ``block/meson.build``
+
+You are done. On build, coroutine wrappers will be generated in
+``<BUILD_DIR>/block/block-gen.c``.
+
+Links
+=====
+
+1. The script location is ``scripts/block-coroutine-wrapper.py``.
+
+2. Generic place for private ``generated_co_wrapper`` declarations is
+   ``block/coroutines.h``, for public declarations:
+   ``include/block/block.h``
+
+3. The core API of generated coroutine wrappers is placed in
+   (not generated) ``block/block-gen.h``
diff --git a/block/block-gen.h b/block/block-gen.h
new file mode 100644
index 0000000000..f80cf4897d
--- /dev/null
+++ b/block/block-gen.h
@@ -0,0 +1,49 @@
+/*
+ * Block coroutine wrapping core, used by auto-generated block/block-gen.c
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2020 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_BLOCK_GEN_H
+#define BLOCK_BLOCK_GEN_H
+
+#include "block/block_int.h"
+
+/* Base structure for argument packing structures */
+typedef struct BdrvPollCo {
+    BlockDriverState *bs;
+    bool in_progress;
+    int ret;
+    Coroutine *co; /* Keep pointer here for debugging */
+} BdrvPollCo;
+
+static inline int bdrv_poll_co(BdrvPollCo *s)
+{
+    assert(!qemu_in_coroutine());
+
+    bdrv_coroutine_enter(s->bs, s->co);
+    BDRV_POLL_WHILE(s->bs, s->in_progress);
+
+    return s->ret;
+}
+
+#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/include/block/block.h b/include/block/block.h
index 8aef849a75..a0655b84d6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -10,6 +10,16 @@
 #include "block/blockjob.h"
 #include "qemu/hbitmap.h"
 
+/*
+ * generated_co_wrapper
+ *
+ * Function specifier, which does nothing but marking functions to be
+ * generated by scripts/block-coroutine-wrapper.py
+ *
+ * Read more in docs/devel/block-coroutine-wrapper.rst
+ */
+#define generated_co_wrapper
+
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 typedef struct BdrvChild BdrvChild;
diff --git a/block/meson.build b/block/meson.build
index a3e56b7cd1..88ad73583a 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
                                command: [module_block_py, '@OUTPUT0@', modsrc])
 block_ss.add(module_block_h)
 
+wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
+block_gen_c = custom_target('block-gen.c',
+                            output: 'block-gen.c',
+                            input: files('../include/block/block.h',
+                                         'coroutines.h'),
+                            command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
+block_ss.add(block_gen_c)
+
 block_ss.add(files('stream.c'))
 
 softmmu_ss.add(files('qapi-sysemu.c'))
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
new file mode 100755
index 0000000000..d859c07a5f
--- /dev/null
+++ b/scripts/block-coroutine-wrapper.py
@@ -0,0 +1,187 @@
+#!/usr/bin/env python3
+"""Generate coroutine wrappers for block subsystem.
+
+The program parses one or several concatenated c files from stdin,
+searches for functions with 'generated_co_wrapper' specifier
+and generates corresponding wrappers on stdout.
+
+Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
+
+Copyright (c) 2020 Virtuozzo International GmbH.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""
+
+import sys
+import re
+import subprocess
+import json
+from typing import Iterator
+
+
+def prettify(code: str) -> str:
+    """Prettify code using clang-format if available"""
+
+    try:
+        style = json.dumps({
+            'IndentWidth': 4,
+            'BraceWrapping': {'AfterFunction': True},
+            'BreakBeforeBraces': 'Custom',
+            'SortIncludes': False,
+            'MaxEmptyLinesToKeep': 2
+        })
+        p = subprocess.run(['clang-format', f'-style={style}'], check=True,
+                           encoding='utf-8', input=code,
+                           stdout=subprocess.PIPE)
+        return p.stdout
+    except FileNotFoundError:
+        return code
+
+
+def gen_header():
+    copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
+    copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
+    copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
+    return f"""\
+/*
+ * File is generated by scripts/block-coroutine-wrapper.py
+ *
+{copyright}
+ */
+
+#include "qemu/osdep.h"
+#include "block/coroutines.h"
+#include "block/block-gen.h"
+#include "block/block_int.h"\
+"""
+
+
+class ParamDecl:
+    param_re = re.compile(r'(?P<decl>'
+                          r'(?P<type>.*[ *])'
+                          r'(?P<name>[a-z][a-z0-9_]*)'
+                          r')')
+
+    def __init__(self, param_decl: str) -> None:
+        m = self.param_re.match(param_decl.strip())
+        if m is None:
+            raise ValueError(f'Wrong parameter declaration: "{param_decl}"')
+        self.decl = m.group('decl')
+        self.type = m.group('type')
+        self.name = m.group('name')
+
+
+class FuncDecl:
+    def __init__(self, return_type: str, name: str, args: str) -> None:
+        self.return_type = return_type.strip()
+        self.name = name.strip()
+        self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+
+    def gen_list(self, format: str) -> str:
+        return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
+
+    def gen_block(self, format: str) -> str:
+        return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
+
+
+# Match wrappers declared with a generated_co_wrapper mark
+func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
+                          r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
+                          r'\((?P<args>[^)]*)\);$', re.MULTILINE)
+
+
+def func_decl_iter(text: str) -> Iterator:
+    for m in func_decl_re.finditer(text):
+        yield FuncDecl(return_type='int',
+                       name=m.group('wrapper_name'),
+                       args=m.group('args'))
+
+
+def snake_to_camel(func_name: str) -> str:
+    """
+    Convert underscore names like 'some_function_name' to camel-case like
+    'SomeFunctionName'
+    """
+    words = func_name.split('_')
+    words = [w[0].upper() + w[1:] for w in words]
+    return ''.join(words)
+
+
+def gen_wrapper(func: FuncDecl) -> str:
+    assert func.name.startswith('bdrv_')
+    assert not func.name.startswith('bdrv_co_')
+    assert func.return_type == 'int'
+    assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
+
+    name = 'bdrv_co_' + func.name[5:]
+    bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
+    struct_name = snake_to_camel(name)
+
+    return f"""\
+/*
+ * Wrappers for {name}
+ */
+
+typedef struct {struct_name} {{
+    BdrvPollCo poll_state;
+{ func.gen_block('    {decl};') }
+}} {struct_name};
+
+static void coroutine_fn {name}_entry(void *opaque)
+{{
+    {struct_name} *s = opaque;
+
+    s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+    s->poll_state.in_progress = false;
+
+    aio_wait_kick();
+}}
+
+int {func.name}({ func.gen_list('{decl}') })
+{{
+    if (qemu_in_coroutine()) {{
+        return {name}({ func.gen_list('{name}') });
+    }} else {{
+        {struct_name} s = {{
+            .poll_state.bs = {bs},
+            .poll_state.in_progress = true,
+
+{ func.gen_block('            .{name} = {name},') }
+        }};
+
+        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+        return bdrv_poll_co(&s.poll_state);
+    }}
+}}"""
+
+
+def gen_wrappers_file(input_code: str) -> str:
+    res = gen_header()
+    for func in func_decl_iter(input_code):
+        res += '\n\n\n'
+        res += gen_wrapper(func)
+
+    return prettify(res)  # prettify to wrap long lines
+
+
+if __name__ == '__main__':
+    if len(sys.argv) < 3:
+        exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
+
+    with open(sys.argv[1], 'w') as f_out:
+        for fname in sys.argv[2:]:
+            with open(fname) as f_in:
+                f_out.write(gen_wrappers_file(f_in.read()))
+                f_out.write('\n')
-- 
2.21.3

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

15.09.2020 19:44, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating coroutine from function
> with several arguments:
> 
>    - create structure to pack parameters
>    - create _entry function to call original function taking parameters
>      from struct
>    - do different magic to handle completion: set ret to NOT_DONE or
>      EINPROGRESS or use separate bool field
>    - fill the struct and create coroutine from _entry function and this
>      struct as a parameter
>    - do coroutine enter and BDRV_POLL_WHILE loop
> 
> Let's reduce code duplication by generating coroutine wrappers.
> 
> This patch adds scripts/block-coroutine-wrapper.py together with some
> friends, which will generate functions with declared prototypes marked
> by 'generated_co_wrapper' specifier.
> 
> The usage of new code generation is as follows:
> 
>      1. define somewhere
> 
>          int coroutine_fn bdrv_co_NAME(...) {...}
> 
>         function
> 
>      2. declare in some header file
> 
>          int generated_co_wrapper bdrv_NAME(...);
> 
>         function with same list of parameters. (you'll need to include
>         "block/generated-co-wrapper.h" to get the specifier)
> 
>      3. both declarations should be available through block/coroutines.h
>         header.
> 
>      4. add header with generated_co_wrapper declaration into
>         COROUTINE_HEADERS list in Makefile
> 
> Still, no function is now marked, this work is for the following
> commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   docs/devel/block-coroutine-wrapper.rst |  54 +++++++
>   block/block-gen.h                      |  49 +++++++
>   include/block/block.h                  |  10 ++
>   block/meson.build                      |   8 ++
>   scripts/block-coroutine-wrapper.py     | 187 +++++++++++++++++++++++++
>   5 files changed, 308 insertions(+)
>   create mode 100644 docs/devel/block-coroutine-wrapper.rst
>   create mode 100644 block/block-gen.h
>   create mode 100755 scripts/block-coroutine-wrapper.py


Also needed:

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076..cb0abe1e69 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
     reset
     s390-dasd-ipl
     clocks
+   block-coroutine-wrapper

-- 
Best regards,
Vladimir

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Eric Blake]

On 9/15/20 3:02 PM, Vladimir Sementsov-Ogievskiy wrote:
> 15.09.2020 19:44, Vladimir Sementsov-Ogievskiy wrote:
>> We have a very frequent pattern of creating coroutine from function
>> with several arguments:
>>
>>    - create structure to pack parameters
>>    - create _entry function to call original function taking parameters
>>      from struct
>>    - do different magic to handle completion: set ret to NOT_DONE or
>>      EINPROGRESS or use separate bool field
>>    - fill the struct and create coroutine from _entry function and this
>>      struct as a parameter
>>    - do coroutine enter and BDRV_POLL_WHILE loop
>>
>> Let's reduce code duplication by generating coroutine wrappers.
>>
>> This patch adds scripts/block-coroutine-wrapper.py together with some
>> friends, which will generate functions with declared prototypes marked
>> by 'generated_co_wrapper' specifier.
>>

>>
>>      4. add header with generated_co_wrapper declaration into
>>         COROUTINE_HEADERS list in Makefile

This phrase is out-of-date.  I also see 4 steps here,...

>>
>> Still, no function is now marked, this work is for the following
>> commit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> ---
>>   docs/devel/block-coroutine-wrapper.rst |  54 +++++++
>>   block/block-gen.h                      |  49 +++++++
>>   include/block/block.h                  |  10 ++
>>   block/meson.build                      |   8 ++
>>   scripts/block-coroutine-wrapper.py     | 187 +++++++++++++++++++++++++
>>   5 files changed, 308 insertions(+)
>>   create mode 100644 docs/devel/block-coroutine-wrapper.rst
>>   create mode 100644 block/block-gen.h
>>   create mode 100755 scripts/block-coroutine-wrapper.py
> 
> 
> Also needed:
> 
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index 04773ce076..cb0abe1e69 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -31,3 +31,4 @@ Contents:
>      reset
>      s390-dasd-ipl
>      clocks
> +   block-coroutine-wrapper

I've squashed that in.

> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -0,0 +1,54 @@
> +=======================
> +block-coroutine-wrapper
> +=======================
> +
> +A lot of functions in QEMJ block layer (see ``block/*``) can by called

QEMU

s/by called only/only be called/

> +only in coroutine context. Such functions are normally marked by

by the

> +coroutine_fn specifier. Still, sometimes we need to call them from
> +non-coroutine context, for this we need to start a coroutine, run the


s/context,/context;/

> +needed function from it and wait for coroutine finish in

in a

> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> +void* argument. So for each coroutine_fn function, which needs

needs a

> +non-coroutine interface, we should define a structure to pack the
> +parameters, define a separate function to unpack the parameters and
> +call the original function and finally define a new interface function
> +with same list of arguments as original one, which will pack the
> +parameters into a struct, create a coroutine, run it and wait in
> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
> +we have a script to generate them.
> 
> +Usage
> +=====
> +
> +Assume we have defined ``coroutine_fn`` function
> +``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
> +called ``bdrv_foo(<same args>)``. In this case the script can help. To
> +trigger the generation:
> +
> +1. You need ``bdrv_foo`` declaration somewhere (for example in
> +   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
> +   like this:

Missing a closing ).

> +
> +.. code-block:: c
> +
> +    int generated_co_wrapper bdrv_foor(<some args>);

s/foor/foo/

> +
> +2. You need to feed this declaration to block-coroutine-wrapper script.

to the block-

> +   For this, add .h (or .c) file with the declaration to
> +   ``input: files(...)`` list of ``block_gen_c`` target declaration in
> +   ``block/meson.build``
> +
> +You are done. On build, coroutine wrappers will be generated in

s/On/During the/

> +``<BUILD_DIR>/block/block-gen.c``.

...but 2 in the .rst.  Presumably, the .rst steps belong in the commit 
message as well.

> +++ b/block/block-gen.h

> +++ b/include/block/block.h
> @@ -10,6 +10,16 @@
>  #include "block/blockjob.h"
>  #include "qemu/hbitmap.h"
>  
> +/*
> + * generated_co_wrapper
> + *
> + * Function specifier, which does nothing but marking functions to be

s/marking/mark/

> + * generated by scripts/block-coroutine-wrapper.py
> + *
> + * Read more in docs/devel/block-coroutine-wrapper.rst
> + */
> +#define generated_co_wrapper
> +
>  /* block.c */
>  typedef struct BlockDriver BlockDriver;
>  typedef struct BdrvChild BdrvChild;
> diff --git a/block/meson.build b/block/meson.build
> index a3e56b7cd1..88ad73583a 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
>                                 command: [module_block_py, '@OUTPUT0@', modsrc])
>  block_ss.add(module_block_h)
>  
> +wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
> +block_gen_c = custom_target('block-gen.c',
> +                            output: 'block-gen.c',
> +                            input: files('../include/block/block.h',
> +                                         'coroutines.h'),
> +                            command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
> +block_ss.add(block_gen_c)
> +
>  block_ss.add(files('stream.c'))

I tested that this works (I'm not a meson expert, but if it works, your 
patch can't be too far off ;)

>  
>  softmmu_ss.add(files('qapi-sysemu.c'))
> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
> new file mode 100755
> index 0000000000..d859c07a5f
> --- /dev/null
> +++ b/scripts/block-coroutine-wrapper.py
> @@ -0,0 +1,187 @@
> +#!/usr/bin/env python3
> +"""Generate coroutine wrappers for block subsystem.
> +
> +The program parses one or several concatenated c files from stdin,
> +searches for functions with 'generated_co_wrapper' specifier

with the

> +and generates corresponding wrappers on stdout.
> +
> +Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
> +
> +Copyright (c) 2020 Virtuozzo International GmbH.
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +"""

John had a patch that unified how copyrights are (or are not) placed in 
doc comments.  I'm of the opinion that it may be easier to apply this 
patch as written and let him touch it up later, rather than forcing it 
to delay longer waiting for his style patches to land first, but I'm 
open to discussion on alternate approaches.

> +
> +import sys
> +import re
> +import subprocess
> +import json
> +from typing import Iterator
> +
> +
> +def prettify(code: str) -> str:
> +    """Prettify code using clang-format if available"""

Nothing else in the codebase uses clang-format, yet.  Is this an 
optional dependency we should be documenting somewhere?

> +
> +    try:
> +        style = json.dumps({
> +            'IndentWidth': 4,
> +            'BraceWrapping': {'AfterFunction': True},
> +            'BreakBeforeBraces': 'Custom',
> +            'SortIncludes': False,
> +            'MaxEmptyLinesToKeep': 2
> +        })

Is it worth always using a trailing comma, so that future additions 
don't have as many lines to touch?

> +        p = subprocess.run(['clang-format', f'-style={style}'], check=True,
> +                           encoding='utf-8', input=code,
> +                           stdout=subprocess.PIPE)
> +        return p.stdout
> +    except FileNotFoundError:
> +        return code
> +
> +
> +def gen_header():
> +    copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
> +    copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
> +    copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
> +    return f"""\
> +/*
> + * File is generated by scripts/block-coroutine-wrapper.py
> + *
> +{copyright}
> + */
> +
> +#include "qemu/osdep.h"
> +#include "block/coroutines.h"
> +#include "block/block-gen.h"
> +#include "block/block_int.h"\
> +"""
> +
> +
> +class ParamDecl:
> +    param_re = re.compile(r'(?P<decl>'
> +                          r'(?P<type>.*[ *])'
> +                          r'(?P<name>[a-z][a-z0-9_]*)'
> +                          r')')
> +
> +    def __init__(self, param_decl: str) -> None:
> +        m = self.param_re.match(param_decl.strip())
> +        if m is None:
> +            raise ValueError(f'Wrong parameter declaration: "{param_decl}"')
> +        self.decl = m.group('decl')
> +        self.type = m.group('type')
> +        self.name = m.group('name')
> +
> +
> +class FuncDecl:
> +    def __init__(self, return_type: str, name: str, args: str) -> None:
> +        self.return_type = return_type.strip()
> +        self.name = name.strip()
> +        self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
> +
> +    def gen_list(self, format: str) -> str:
> +        return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> +
> +    def gen_block(self, format: str) -> str:
> +        return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
> +
> +
> +# Match wrappers declared with a generated_co_wrapper mark
> +func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
> +                          r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
> +                          r'\((?P<args>[^)]*)\);$', re.MULTILINE)
> +
> +
> +def func_decl_iter(text: str) -> Iterator:
> +    for m in func_decl_re.finditer(text):
> +        yield FuncDecl(return_type='int',
> +                       name=m.group('wrapper_name'),
> +                       args=m.group('args'))
> +
> +
> +def snake_to_camel(func_name: str) -> str:
> +    """
> +    Convert underscore names like 'some_function_name' to camel-case like
> +    'SomeFunctionName'
> +    """
> +    words = func_name.split('_')
> +    words = [w[0].upper() + w[1:] for w in words]
> +    return ''.join(words)
> +
> +
> +def gen_wrapper(func: FuncDecl) -> str:
> +    assert func.name.startswith('bdrv_')
> +    assert not func.name.startswith('bdrv_co_')
> +    assert func.return_type == 'int'
> +    assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
> +
> +    name = 'bdrv_co_' + func.name[5:]
> +    bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
> +    struct_name = snake_to_camel(name)
> +
> +    return f"""\
> +/*
> + * Wrappers for {name}
> + */
> +
> +typedef struct {struct_name} {{
> +    BdrvPollCo poll_state;
> +{ func.gen_block('    {decl};') }
> +}} {struct_name};
> +
> +static void coroutine_fn {name}_entry(void *opaque)
> +{{
> +    {struct_name} *s = opaque;
> +
> +    s->poll_state.ret = {name}({ func.gen_list('s->{name}') });

Excuse my lack of python style awareness, but why are we mixing 
{nospace} and { space } on the same line?

> +    s->poll_state.in_progress = false;
> +
> +    aio_wait_kick();
> +}}
> +
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> +    if (qemu_in_coroutine()) {{
> +        return {name}({ func.gen_list('{name}') });
> +    }} else {{
> +        {struct_name} s = {{
> +            .poll_state.bs = {bs},
> +            .poll_state.in_progress = true,
> +
> +{ func.gen_block('            .{name} = {name},') }
> +        }};
> +
> +        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> +        return bdrv_poll_co(&s.poll_state);
> +    }}
> +}}"""
> +
> +
> +def gen_wrappers_file(input_code: str) -> str:
> +    res = gen_header()
> +    for func in func_decl_iter(input_code):
> +        res += '\n\n\n'
> +        res += gen_wrapper(func)
> +
> +    return prettify(res)  # prettify to wrap long lines
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) < 3:
> +        exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
> +
> +    with open(sys.argv[1], 'w') as f_out:
> +        for fname in sys.argv[2:]:
> +            with open(fname) as f_in:
> +                f_out.write(gen_wrappers_file(f_in.read()))
> +                f_out.write('\n')

Tested-by: Eric Blake <eblake@redhat.com>

There's enough grammar fixes, and the fact that John is working on 
python cleanups, to make me wonder if we need a v9, or if I should just 
stage it where it is with any other cleanups as followups.  But I'm 
liking the reduced maintenance burden once it is in, and don't want to 
drag it out to the point that it needs more rebasing as other things 
land first.

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

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Eric Blake]

On 9/23/20 7:00 PM, Eric Blake wrote:

> 
> Tested-by: Eric Blake <eblake@redhat.com>
> 
> There's enough grammar fixes, and the fact that John is working on 
> python cleanups, to make me wonder if we need a v9, or if I should just 
> stage it where it is with any other cleanups as followups.  But I'm 
> liking the reduced maintenance burden once it is in, and don't want to 
> drag it out to the point that it needs more rebasing as other things 
> land first.
> 

Here's what I've squashed in and temporarily pushed to my tree if you 
want to double-check my rebase work:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/master

diff --git a/docs/devel/block-coroutine-wrapper.rst 
b/docs/devel/block-coroutine-wrapper.rst
index f7050bbc8fa6..d09fff2cc539 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -2,43 +2,43 @@
  block-coroutine-wrapper
  =======================

-A lot of functions in QEMJ block layer (see ``block/*``) can by called
-only in coroutine context. Such functions are normally marked by
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
  coroutine_fn specifier. Still, sometimes we need to call them from
-non-coroutine context, for this we need to start a coroutine, run the
+non-coroutine context; for this we need to start a coroutine, run the
  needed function from it and wait for coroutine finish in
  BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
-void* argument. So for each coroutine_fn function, which needs
+void* argument. So for each coroutine_fn function which needs a
  non-coroutine interface, we should define a structure to pack the
  parameters, define a separate function to unpack the parameters and
  call the original function and finally define a new interface function
  with same list of arguments as original one, which will pack the
  parameters into a struct, create a coroutine, run it and wait in
-BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
-we have a script to generate them.
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.

  Usage
  =====

-Assume we have defined ``coroutine_fn`` function
+Assume we have defined the ``coroutine_fn`` function
  ``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
  called ``bdrv_foo(<same args>)``. In this case the script can help. To
  trigger the generation:

-1. You need ``bdrv_foo`` declaration somewhere (for example in
-   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
+1. You need ``bdrv_foo`` declaration somewhere (for example, in
+   ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
     like this:

  .. code-block:: c

-    int generated_co_wrapper bdrv_foor(<some args>);
+    int generated_co_wrapper bdrv_foo(<some args>);

  2. You need to feed this declaration to block-coroutine-wrapper script.
-   For this, add .h (or .c) file with the declaration to
+   For this, add the .h (or .c) file with the declaration to the
     ``input: files(...)`` list of ``block_gen_c`` target declaration in
     ``block/meson.build``

-You are done. On build, coroutine wrappers will be generated in
+You are done. During the build, coroutine wrappers will be generated in
  ``<BUILD_DIR>/block/block-gen.c``.

  Links
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076b3..cb0abe1e6988 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
     reset
     s390-dasd-ipl
     clocks
+   block-coroutine-wrapper
diff --git a/scripts/block-coroutine-wrapper.py 
b/scripts/block-coroutine-wrapper.py
index d859c07a5f55..8c0a08d9b020 100755
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
  """Generate coroutine wrappers for block subsystem.

  The program parses one or several concatenated c files from stdin,
-searches for functions with 'generated_co_wrapper' specifier
+searches for functions with the 'generated_co_wrapper' specifier
  and generates corresponding wrappers on stdout.

  Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -39,7 +39,7 @@ def prettify(code: str) -> str:
              'BraceWrapping': {'AfterFunction': True},
              'BreakBeforeBraces': 'Custom',
              'SortIncludes': False,
-            'MaxEmptyLinesToKeep': 2
+            'MaxEmptyLinesToKeep': 2,
          })
          p = subprocess.run(['clang-format', f'-style={style}'], 
check=True,
                             encoding='utf-8', input=code,
@@ -168,7 +168,7 @@ int {func.name}({ func.gen_list('{decl}') })


  def gen_wrappers_file(input_code: str) -> str:
-    res = gen_header()
+    res = ''
      for func in func_decl_iter(input_code):
          res += '\n\n\n'
          res += gen_wrapper(func)
@@ -181,6 +181,7 @@ if __name__ == '__main__':
          exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')

      with open(sys.argv[1], 'w') as f_out:
+        f_out.write(gen_header())
          for fname in sys.argv[2:]:
              with open(fname) as f_in:
                  f_out.write(gen_wrappers_file(f_in.read()))
diff --git a/include/block/block.h b/include/block/block.h
index d861883b8d9e..0f0ddc51b49e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -13,7 +13,7 @@
  /*
   * generated_co_wrapper
   *
- * Function specifier, which does nothing but marking functions to be
+ * Function specifier, which does nothing but mark functions to be
   * generated by scripts/block-coroutine-wrapper.py
   *
   * Read more in docs/devel/block-coroutine-wrapper.rst



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

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

24.09.2020 04:20, Eric Blake wrote:
> On 9/23/20 7:00 PM, Eric Blake wrote:
> 
>>
>> Tested-by: Eric Blake <eblake@redhat.com>
>>
>> There's enough grammar fixes, and the fact that John is working on python cleanups, to make me wonder if we need a v9, or if I should just stage it where it is with any other cleanups as followups.  But I'm liking the reduced maintenance burden once it is in, and don't want to drag it out to the point that it needs more rebasing as other things land first.
>>
> 
> Here's what I've squashed in and temporarily pushed to my tree if you want to double-check my rebase work:
> https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/master
> 
> diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
> index f7050bbc8fa6..d09fff2cc539 100644
> --- a/docs/devel/block-coroutine-wrapper.rst
> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -2,43 +2,43 @@
>   block-coroutine-wrapper
>   =======================
> 
> -A lot of functions in QEMJ block layer (see ``block/*``) can by called
> -only in coroutine context. Such functions are normally marked by
> +A lot of functions in QEMU block layer (see ``block/*``) can only be
> +called in coroutine context. Such functions are normally marked by the
>   coroutine_fn specifier. Still, sometimes we need to call them from
> -non-coroutine context, for this we need to start a coroutine, run the
> +non-coroutine context; for this we need to start a coroutine, run the
>   needed function from it and wait for coroutine finish in
>   BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> -void* argument. So for each coroutine_fn function, which needs
> +void* argument. So for each coroutine_fn function which needs a
>   non-coroutine interface, we should define a structure to pack the
>   parameters, define a separate function to unpack the parameters and
>   call the original function and finally define a new interface function
>   with same list of arguments as original one, which will pack the
>   parameters into a struct, create a coroutine, run it and wait in
> -BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
> -we have a script to generate them.
> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
> +so we have a script to generate them.
> 
>   Usage
>   =====
> 
> -Assume we have defined ``coroutine_fn`` function
> +Assume we have defined the ``coroutine_fn`` function
>   ``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
>   called ``bdrv_foo(<same args>)``. In this case the script can help. To
>   trigger the generation:
> 
> -1. You need ``bdrv_foo`` declaration somewhere (for example in
> -   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
> +1. You need ``bdrv_foo`` declaration somewhere (for example, in
> +   ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
>      like this:
> 
>   .. code-block:: c
> 
> -    int generated_co_wrapper bdrv_foor(<some args>);
> +    int generated_co_wrapper bdrv_foo(<some args>);
> 
>   2. You need to feed this declaration to block-coroutine-wrapper script.
> -   For this, add .h (or .c) file with the declaration to
> +   For this, add the .h (or .c) file with the declaration to the
>      ``input: files(...)`` list of ``block_gen_c`` target declaration in
>      ``block/meson.build``
> 
> -You are done. On build, coroutine wrappers will be generated in
> +You are done. During the build, coroutine wrappers will be generated in
>   ``<BUILD_DIR>/block/block-gen.c``.
> 
>   Links
> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
> index 04773ce076b3..cb0abe1e6988 100644
> --- a/docs/devel/index.rst
> +++ b/docs/devel/index.rst
> @@ -31,3 +31,4 @@ Contents:
>      reset
>      s390-dasd-ipl
>      clocks
> +   block-coroutine-wrapper
> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
> index d859c07a5f55..8c0a08d9b020 100755
> --- a/scripts/block-coroutine-wrapper.py
> +++ b/scripts/block-coroutine-wrapper.py
> @@ -2,7 +2,7 @@
>   """Generate coroutine wrappers for block subsystem.
> 
>   The program parses one or several concatenated c files from stdin,
> -searches for functions with 'generated_co_wrapper' specifier
> +searches for functions with the 'generated_co_wrapper' specifier
>   and generates corresponding wrappers on stdout.
> 
>   Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
> @@ -39,7 +39,7 @@ def prettify(code: str) -> str:
>               'BraceWrapping': {'AfterFunction': True},
>               'BreakBeforeBraces': 'Custom',
>               'SortIncludes': False,
> -            'MaxEmptyLinesToKeep': 2
> +            'MaxEmptyLinesToKeep': 2,
>           })
>           p = subprocess.run(['clang-format', f'-style={style}'], check=True,
>                              encoding='utf-8', input=code,
> @@ -168,7 +168,7 @@ int {func.name}({ func.gen_list('{decl}') })
> 
> 
>   def gen_wrappers_file(input_code: str) -> str:
> -    res = gen_header()
> +    res = ''
>       for func in func_decl_iter(input_code):
>           res += '\n\n\n'
>           res += gen_wrapper(func)
> @@ -181,6 +181,7 @@ if __name__ == '__main__':
>           exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
> 
>       with open(sys.argv[1], 'w') as f_out:
> +        f_out.write(gen_header())
>           for fname in sys.argv[2:]:
>               with open(fname) as f_in:
>                   f_out.write(gen_wrappers_file(f_in.read()))
> diff --git a/include/block/block.h b/include/block/block.h
> index d861883b8d9e..0f0ddc51b49e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -13,7 +13,7 @@
>   /*
>    * generated_co_wrapper
>    *
> - * Function specifier, which does nothing but marking functions to be
> + * Function specifier, which does nothing but mark functions to be
>    * generated by scripts/block-coroutine-wrapper.py
>    *
>    * Read more in docs/devel/block-coroutine-wrapper.rst
> 
> 
> 


Seems OK, thanks!

-- 
Best regards,
Vladimir

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

24.09.2020 03:00, Eric Blake wrote:
> On 9/15/20 3:02 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.09.2020 19:44, Vladimir Sementsov-Ogievskiy wrote:
>>> We have a very frequent pattern of creating coroutine from function
>>> with several arguments:
>>>
>>>    - create structure to pack parameters
>>>    - create _entry function to call original function taking parameters
>>>      from struct
>>>    - do different magic to handle completion: set ret to NOT_DONE or
>>>      EINPROGRESS or use separate bool field
>>>    - fill the struct and create coroutine from _entry function and this
>>>      struct as a parameter
>>>    - do coroutine enter and BDRV_POLL_WHILE loop
>>>
>>> Let's reduce code duplication by generating coroutine wrappers.
>>>
>>> This patch adds scripts/block-coroutine-wrapper.py together with some
>>> friends, which will generate functions with declared prototypes marked
>>> by 'generated_co_wrapper' specifier.
>>>
> 
>>>
>>>      4. add header with generated_co_wrapper declaration into
>>>         COROUTINE_HEADERS list in Makefile
> 
> This phrase is out-of-date.  I also see 4 steps here,...
> 
>>>
>>> Still, no function is now marked, this work is for the following
>>> commit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>> ---
>>>   docs/devel/block-coroutine-wrapper.rst |  54 +++++++
>>>   block/block-gen.h                      |  49 +++++++
>>>   include/block/block.h                  |  10 ++
>>>   block/meson.build                      |   8 ++
>>>   scripts/block-coroutine-wrapper.py     | 187 +++++++++++++++++++++++++
>>>   5 files changed, 308 insertions(+)
>>>   create mode 100644 docs/devel/block-coroutine-wrapper.rst
>>>   create mode 100644 block/block-gen.h
>>>   create mode 100755 scripts/block-coroutine-wrapper.py
>>
>>
>> Also needed:
>>
>> diff --git a/docs/devel/index.rst b/docs/devel/index.rst
>> index 04773ce076..cb0abe1e69 100644
>> --- a/docs/devel/index.rst
>> +++ b/docs/devel/index.rst
>> @@ -31,3 +31,4 @@ Contents:
>>      reset
>>      s390-dasd-ipl
>>      clocks
>> +   block-coroutine-wrapper
> 
> I've squashed that in.
> 
>> +++ b/docs/devel/block-coroutine-wrapper.rst
>> @@ -0,0 +1,54 @@
>> +=======================
>> +block-coroutine-wrapper
>> +=======================
>> +
>> +A lot of functions in QEMJ block layer (see ``block/*``) can by called
> 
> QEMU
> 
> s/by called only/only be called/
> 
>> +only in coroutine context. Such functions are normally marked by
> 
> by the
> 
>> +coroutine_fn specifier. Still, sometimes we need to call them from
>> +non-coroutine context, for this we need to start a coroutine, run the
> 
> 
> s/context,/context;/
> 
>> +needed function from it and wait for coroutine finish in
> 
> in a
> 
>> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
>> +void* argument. So for each coroutine_fn function, which needs
> 
> needs a
> 
>> +non-coroutine interface, we should define a structure to pack the
>> +parameters, define a separate function to unpack the parameters and
>> +call the original function and finally define a new interface function
>> +with same list of arguments as original one, which will pack the
>> +parameters into a struct, create a coroutine, run it and wait in
>> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
>> +we have a script to generate them.
>>
>> +Usage
>> +=====
>> +
>> +Assume we have defined ``coroutine_fn`` function
>> +``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
>> +called ``bdrv_foo(<same args>)``. In this case the script can help. To
>> +trigger the generation:
>> +
>> +1. You need ``bdrv_foo`` declaration somewhere (for example in
>> +   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
>> +   like this:
> 
> Missing a closing ).
> 
>> +
>> +.. code-block:: c
>> +
>> +    int generated_co_wrapper bdrv_foor(<some args>);
> 
> s/foor/foo/
> 
>> +
>> +2. You need to feed this declaration to block-coroutine-wrapper script.
> 
> to the block-
> 
>> +   For this, add .h (or .c) file with the declaration to
>> +   ``input: files(...)`` list of ``block_gen_c`` target declaration in
>> +   ``block/meson.build``
>> +
>> +You are done. On build, coroutine wrappers will be generated in
> 
> s/On/During the/
> 
>> +``<BUILD_DIR>/block/block-gen.c``.
> 
> ...but 2 in the .rst.  Presumably, the .rst steps belong in the commit message as well.
> 
>> +++ b/block/block-gen.h
> 
>> +++ b/include/block/block.h
>> @@ -10,6 +10,16 @@
>>  #include "block/blockjob.h"
>>  #include "qemu/hbitmap.h"
>>
>> +/*
>> + * generated_co_wrapper
>> + *
>> + * Function specifier, which does nothing but marking functions to be
> 
> s/marking/mark/
> 
>> + * generated by scripts/block-coroutine-wrapper.py
>> + *
>> + * Read more in docs/devel/block-coroutine-wrapper.rst
>> + */
>> +#define generated_co_wrapper
>> +
>>  /* block.c */
>>  typedef struct BlockDriver BlockDriver;
>>  typedef struct BdrvChild BdrvChild;
>> diff --git a/block/meson.build b/block/meson.build
>> index a3e56b7cd1..88ad73583a 100644
>> --- a/block/meson.build
>> +++ b/block/meson.build
>> @@ -107,6 +107,14 @@ module_block_h = custom_target('module_block.h',
>>                                 command: [module_block_py, '@OUTPUT0@', modsrc])
>>  block_ss.add(module_block_h)
>>
>> +wrapper_py = find_program('../scripts/block-coroutine-wrapper.py')
>> +block_gen_c = custom_target('block-gen.c',
>> +                            output: 'block-gen.c',
>> +                            input: files('../include/block/block.h',
>> +                                         'coroutines.h'),
>> +                            command: [wrapper_py, '@OUTPUT@', '@INPUT@'])
>> +block_ss.add(block_gen_c)
>> +
>>  block_ss.add(files('stream.c'))
> 
> I tested that this works (I'm not a meson expert, but if it works, your patch can't be too far off ;)
> 
>>
>>  softmmu_ss.add(files('qapi-sysemu.c'))
>> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
>> new file mode 100755
>> index 0000000000..d859c07a5f
>> --- /dev/null
>> +++ b/scripts/block-coroutine-wrapper.py
>> @@ -0,0 +1,187 @@
>> +#!/usr/bin/env python3
>> +"""Generate coroutine wrappers for block subsystem.
>> +
>> +The program parses one or several concatenated c files from stdin,
>> +searches for functions with 'generated_co_wrapper' specifier
> 
> with the
> 
>> +and generates corresponding wrappers on stdout.
>> +
>> +Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
>> +
>> +Copyright (c) 2020 Virtuozzo International GmbH.
>> +
>> +This program is free software; you can redistribute it and/or modify
>> +it under the terms of the GNU General Public License as published by
>> +the Free Software Foundation; either version 2 of the License, or
>> +(at your option) any later version.
>> +
>> +This program is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +GNU General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +"""
> 
> John had a patch that unified how copyrights are (or are not) placed in doc comments.  I'm of the opinion that it may be easier to apply this patch as written and let him touch it up later, rather than forcing it to delay longer waiting for his style patches to land first, but I'm open to discussion on alternate approaches.

Here copyright is in doc string for purpose: it's used also for generated files :) Not sure how much is it correct, but it's fun.

> 
>> +
>> +import sys
>> +import re
>> +import subprocess
>> +import json
>> +from typing import Iterator
>> +
>> +
>> +def prettify(code: str) -> str:
>> +    """Prettify code using clang-format if available"""
> 
> Nothing else in the codebase uses clang-format, yet.  Is this an optional dependency we should be documenting somewhere?

Hmm at least I should note it in docs/devel/block-coroutine-wrapper.rst

> 
>> +
>> +    try:
>> +        style = json.dumps({
>> +            'IndentWidth': 4,
>> +            'BraceWrapping': {'AfterFunction': True},
>> +            'BreakBeforeBraces': 'Custom',
>> +            'SortIncludes': False,
>> +            'MaxEmptyLinesToKeep': 2
>> +        })
> 
> Is it worth always using a trailing comma, so that future additions don't have as many lines to touch?

Yes

> 
>> +        p = subprocess.run(['clang-format', f'-style={style}'], check=True,
>> +                           encoding='utf-8', input=code,
>> +                           stdout=subprocess.PIPE)
>> +        return p.stdout
>> +    except FileNotFoundError:
>> +        return code
>> +
>> +
>> +def gen_header():
>> +    copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
>> +    copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
>> +    copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
>> +    return f"""\
>> +/*
>> + * File is generated by scripts/block-coroutine-wrapper.py
>> + *
>> +{copyright}
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "block/coroutines.h"
>> +#include "block/block-gen.h"
>> +#include "block/block_int.h"\
>> +"""
>> +
>> +
>> +class ParamDecl:
>> +    param_re = re.compile(r'(?P<decl>'
>> +                          r'(?P<type>.*[ *])'
>> +                          r'(?P<name>[a-z][a-z0-9_]*)'
>> +                          r')')
>> +
>> +    def __init__(self, param_decl: str) -> None:
>> +        m = self.param_re.match(param_decl.strip())
>> +        if m is None:
>> +            raise ValueError(f'Wrong parameter declaration: "{param_decl}"')
>> +        self.decl = m.group('decl')
>> +        self.type = m.group('type')
>> +        self.name = m.group('name')
>> +
>> +
>> +class FuncDecl:
>> +    def __init__(self, return_type: str, name: str, args: str) -> None:
>> +        self.return_type = return_type.strip()
>> +        self.name = name.strip()
>> +        self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
>> +
>> +    def gen_list(self, format: str) -> str:
>> +        return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
>> +
>> +    def gen_block(self, format: str) -> str:
>> +        return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
>> +
>> +
>> +# Match wrappers declared with a generated_co_wrapper mark
>> +func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
>> +                          r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
>> +                          r'\((?P<args>[^)]*)\);$', re.MULTILINE)
>> +
>> +
>> +def func_decl_iter(text: str) -> Iterator:
>> +    for m in func_decl_re.finditer(text):
>> +        yield FuncDecl(return_type='int',
>> +                       name=m.group('wrapper_name'),
>> +                       args=m.group('args'))
>> +
>> +
>> +def snake_to_camel(func_name: str) -> str:
>> +    """
>> +    Convert underscore names like 'some_function_name' to camel-case like
>> +    'SomeFunctionName'
>> +    """
>> +    words = func_name.split('_')
>> +    words = [w[0].upper() + w[1:] for w in words]
>> +    return ''.join(words)
>> +
>> +
>> +def gen_wrapper(func: FuncDecl) -> str:
>> +    assert func.name.startswith('bdrv_')
>> +    assert not func.name.startswith('bdrv_co_')
>> +    assert func.return_type == 'int'
>> +    assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
>> +
>> +    name = 'bdrv_co_' + func.name[5:]
>> +    bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
>> +    struct_name = snake_to_camel(name)
>> +
>> +    return f"""\
>> +/*
>> + * Wrappers for {name}
>> + */
>> +
>> +typedef struct {struct_name} {{
>> +    BdrvPollCo poll_state;
>> +{ func.gen_block('    {decl};') }
>> +}} {struct_name};
>> +
>> +static void coroutine_fn {name}_entry(void *opaque)
>> +{{
>> +    {struct_name} *s = opaque;
>> +
>> +    s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
> 
> Excuse my lack of python style awareness, but why are we mixing {nospace} and { space } on the same line?

Hmm. It's just
  - strange for me to add spaces in first case, for just one variable
  - hard to read the second case without spaces
  - strange to make exclusion for only one line (if add spaces in both cases here, we should add spaces in all cases in the file)

It seems inconsistent.. But at least I'm consistent in this mixed style:) So, I'd keep as if neither of us has strong opinion on this.

> 
>> +    s->poll_state.in_progress = false;
>> +
>> +    aio_wait_kick();
>> +}}
>> +
>> +int {func.name}({ func.gen_list('{decl}') })
>> +{{
>> +    if (qemu_in_coroutine()) {{
>> +        return {name}({ func.gen_list('{name}') });
>> +    }} else {{
>> +        {struct_name} s = {{
>> +            .poll_state.bs = {bs},
>> +            .poll_state.in_progress = true,
>> +
>> +{ func.gen_block('            .{name} = {name},') }
>> +        }};
>> +
>> +        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
>> +
>> +        return bdrv_poll_co(&s.poll_state);
>> +    }}
>> +}}"""
>> +
>> +
>> +def gen_wrappers_file(input_code: str) -> str:
>> +    res = gen_header()
>> +    for func in func_decl_iter(input_code):
>> +        res += '\n\n\n'
>> +        res += gen_wrapper(func)
>> +
>> +    return prettify(res)  # prettify to wrap long lines
>> +
>> +
>> +if __name__ == '__main__':
>> +    if len(sys.argv) < 3:
>> +        exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
>> +
>> +    with open(sys.argv[1], 'w') as f_out:
>> +        for fname in sys.argv[2:]:
>> +            with open(fname) as f_in:
>> +                f_out.write(gen_wrappers_file(f_in.read()))
>> +                f_out.write('\n')
> 
> Tested-by: Eric Blake <eblake@redhat.com>
> 
> There's enough grammar fixes, and the fact that John is working on python cleanups, to make me wonder if we need a v9, or if I should just stage it where it is with any other cleanups as followups.  But I'm liking the reduced maintenance burden once it is in, and don't want to drag it out to the point that it needs more rebasing as other things land first.
> 


-- 
Best regards,
Vladimir

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[John Snow]

On 9/23/20 8:00 PM, Eric Blake wrote:
> 
> 
> There's enough grammar fixes, and the fact that John is working on 
> python cleanups, to make me wonder if we need a v9, or if I should just 
> stage it where it is with any other cleanups as followups.  But I'm 
> liking the reduced maintenance burden once it is in, and don't want to 
> drag it out to the point that it needs more rebasing as other things 
> land first.

Don't worry about it too much. I'd rather we have a helpful script 
sooner than later than worry about Python style purity before I have the 
CI mechanisms for it fully established.

(I eyeballed it and it looks like you already use type hints, so I have 
faith it won't need much cleanup.)

Thanks!

--js

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Eric Blake]

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating coroutine from function
> with several arguments:
> 

> +++ b/scripts/block-coroutine-wrapper.py
> @@ -0,0 +1,187 @@
> +#!/usr/bin/env python3
> +"""Generate coroutine wrappers for block subsystem.

Looking at the generated file after patch 5 is applied,...


> +
> +def gen_header():
> +    copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
> +    copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
> +    copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
> +    return f"""\

This generated comment...


> +
> +
> +def gen_wrappers_file(input_code: str) -> str:
> +    res = gen_header()

...is getting inserted into the generated file...

> +    for func in func_decl_iter(input_code):
> +        res += '\n\n\n'
> +        res += gen_wrapper(func)
> +
> +    return prettify(res)  # prettify to wrap long lines
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) < 3:
> +        exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
> +
> +    with open(sys.argv[1], 'w') as f_out:
> +        for fname in sys.argv[2:]:
> +            with open(fname) as f_in:
> +                f_out.write(gen_wrappers_file(f_in.read()))

multiple times.  You'll want to hoist the call to gen_header outside the 
loop over fname in sys.argv[2:].

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

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

24.09.2020 03:18, Eric Blake wrote:
> On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> We have a very frequent pattern of creating coroutine from function
>> with several arguments:
>>
> 
>> +++ b/scripts/block-coroutine-wrapper.py
>> @@ -0,0 +1,187 @@
>> +#!/usr/bin/env python3
>> +"""Generate coroutine wrappers for block subsystem.
> 
> Looking at the generated file after patch 5 is applied,...
> 
> 
>> +
>> +def gen_header():
>> +    copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
>> +    copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
>> +    copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
>> +    return f"""\
> 
> This generated comment...
> 
> 
>> +
>> +
>> +def gen_wrappers_file(input_code: str) -> str:
>> +    res = gen_header()
> 
> ...is getting inserted into the generated file...
> 
>> +    for func in func_decl_iter(input_code):
>> +        res += '\n\n\n'
>> +        res += gen_wrapper(func)
>> +
>> +    return prettify(res)  # prettify to wrap long lines
>> +
>> +
>> +if __name__ == '__main__':
>> +    if len(sys.argv) < 3:
>> +        exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
>> +
>> +    with open(sys.argv[1], 'w') as f_out:
>> +        for fname in sys.argv[2:]:
>> +            with open(fname) as f_in:
>> +                f_out.write(gen_wrappers_file(f_in.read()))
> 
> multiple times.  You'll want to hoist the call to gen_header outside the loop over fname in sys.argv[2:].
> 

Right, thanks for fixing. I missed it when rebasing on meson system (and move to calling gen_wrappers_file() several times). Hmm, gen_wrappers_file() is now a bit misleading name, it would better be just gen_wrappers()

-- 
Best regards,
Vladimir

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>  create mode 100755 scripts/block-coroutine-wrapper.py

Please see docs/devel/build-system.rst "Support scripts" for the
preferred way of adding Python scripts to the build system. Mode should
be 644 and the interpreter line should be "#! /usr/bin/env python3"
(with the space). That way meson will run it under the configured
--python= interpreter.

> 
> diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
> new file mode 100644
> index 0000000000..f7050bbc8f
> --- /dev/null
> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -0,0 +1,54 @@
> +=======================
> +block-coroutine-wrapper
> +=======================
> +
> +A lot of functions in QEMJ block layer (see ``block/*``) can by called

s/QEMJ/QEMU/

> +only in coroutine context. Such functions are normally marked by
> +coroutine_fn specifier. Still, sometimes we need to call them from
> +non-coroutine context, for this we need to start a coroutine, run the
> +needed function from it and wait for coroutine finish in
> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> +void* argument. So for each coroutine_fn function, which needs
> +non-coroutine interface, we should define a structure to pack the
> +parameters, define a separate function to unpack the parameters and
> +call the original function and finally define a new interface function
> +with same list of arguments as original one, which will pack the
> +parameters into a struct, create a coroutine, run it and wait in
> +BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
> +we have a script to generate them.
> +
> +Usage
> +=====
> +
> +Assume we have defined ``coroutine_fn`` function
> +``bdrv_co_foo(<some args>)`` and need a non-coroutine interface for it,
> +called ``bdrv_foo(<same args>)``. In this case the script can help. To
> +trigger the generation:
> +
> +1. You need ``bdrv_foo`` declaration somewhere (for example in
> +   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
> +   like this:
> +
> +.. code-block:: c
> +
> +    int generated_co_wrapper bdrv_foor(<some args>);

s/foor/foo/

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

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Eric Blake]

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> We have a very frequent pattern of creating coroutine from function
> with several arguments:
> 

> +++ b/docs/devel/block-coroutine-wrapper.rst
> @@ -0,0 +1,54 @@
> +=======================
> +block-coroutine-wrapper
> +=======================
> +
> +A lot of functions in QEMJ block layer (see ``block/*``) can by called

My editor italicized everhting after block/*...

> +only in coroutine context. Such functions are normally marked by
> +coroutine_fn specifier. Still, sometimes we need to call them from
> +non-coroutine context, for this we need to start a coroutine, run the
> +needed function from it and wait for coroutine finish in
> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
> +void* argument. So for each coroutine_fn function, which needs

...through void*.  I wonder if you need to use \* to let .rst know that 
these are literal *, and not markup for a different font style. 
Although I did not check the actual generated docs to see how they look.

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

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

[Vladimir Sementsov-Ogievskiy]

24.09.2020 20:56, Eric Blake wrote:
> On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> We have a very frequent pattern of creating coroutine from function
>> with several arguments:
>>
> 
>> +++ b/docs/devel/block-coroutine-wrapper.rst
>> @@ -0,0 +1,54 @@
>> +=======================
>> +block-coroutine-wrapper
>> +=======================
>> +
>> +A lot of functions in QEMJ block layer (see ``block/*``) can by called
> 
> My editor italicized everhting after block/*...
> 
>> +only in coroutine context. Such functions are normally marked by
>> +coroutine_fn specifier. Still, sometimes we need to call them from
>> +non-coroutine context, for this we need to start a coroutine, run the
>> +needed function from it and wait for coroutine finish in
>> +BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
>> +void* argument. So for each coroutine_fn function, which needs
> 
> ...through void*.  I wonder if you need to use \* to let .rst know that these are literal *, and not markup for a different font style. Although I did not check the actual generated docs to see how they look.
> 

Intuitively, `` should have greater priority than *.

I now check ./build/docs/devel/block-coroutine-wrapper.html , it looks OK:

    A lot of functions in QEMU block layer (see <code class="docutils literal notranslate"><span class="pre">block/*</span></code>) can only be

-- 
Best regards,
Vladimir

[PATCH v8 5/7] block: generate coroutine-wrapper code

[Vladimir Sementsov-Ogievskiy]

Use code generation implemented in previous commit to generated
coroutine wrappers in block.c and block/io.c

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/coroutines.h    |   6 +-
 include/block/block.h |  16 ++--
 block.c               |  73 ---------------
 block/io.c            | 212 ------------------------------------------
 4 files changed, 13 insertions(+), 294 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h
index 9ce1730a09..c62b3a2697 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -34,7 +34,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
 int coroutine_fn
 bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
              bool is_write, BdrvRequestFlags flags);
-int
+int generated_co_wrapper
 bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
           bool is_write, BdrvRequestFlags flags);
 
@@ -47,7 +47,7 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
                                   int64_t *pnum,
                                   int64_t *map,
                                   BlockDriverState **file);
-int
+int generated_co_wrapper
 bdrv_common_block_status_above(BlockDriverState *bs,
                                BlockDriverState *base,
                                bool want_zero,
@@ -60,7 +60,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
 int coroutine_fn
 bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
                    bool is_read);
-int
+int generated_co_wrapper
 bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
                 bool is_read);
 
diff --git a/include/block/block.h b/include/block/block.h
index a0655b84d6..d8fb02fa2a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -403,8 +403,9 @@ void bdrv_refresh_filename(BlockDriverState *bs);
 int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
                                   PreallocMode prealloc, BdrvRequestFlags flags,
                                   Error **errp);
-int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-                  PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
+int generated_co_wrapper
+bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
+              PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
 
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
@@ -446,7 +447,8 @@ typedef enum {
     BDRV_FIX_ERRORS   = 2,
 } BdrvCheckMode;
 
-int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
+int generated_co_wrapper bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
+                                    BdrvCheckMode fix);
 
 /* The units of offset and total_work_size may be chosen arbitrarily by the
  * block driver; total_work_size may change during the course of the amendment
@@ -470,12 +472,13 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb);
 int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
 
 /* Invalidate any cached metadata used by image formats */
-int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
+int generated_co_wrapper bdrv_invalidate_cache(BlockDriverState *bs,
+                                               Error **errp);
 void bdrv_invalidate_cache_all(Error **errp);
 int bdrv_inactivate_all(void);
 
 /* Ensure contents are flushed to disk.  */
-int bdrv_flush(BlockDriverState *bs);
+int generated_co_wrapper bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
 int bdrv_flush_all(void);
 void bdrv_close_all(void);
@@ -490,7 +493,8 @@ void bdrv_drain_all(void);
     AIO_WAIT_WHILE(bdrv_get_aio_context(bs_),              \
                    cond); })
 
-int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
+int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
+                                       int64_t bytes);
 int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ec5a8cbd7b..d49d591917 100644
--- a/block.c
+++ b/block.c
@@ -4655,43 +4655,6 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
     return bs->drv->bdrv_co_check(bs, res, fix);
 }
 
-typedef struct CheckCo {
-    BlockDriverState *bs;
-    BdrvCheckResult *res;
-    BdrvCheckMode fix;
-    int ret;
-} CheckCo;
-
-static void coroutine_fn bdrv_check_co_entry(void *opaque)
-{
-    CheckCo *cco = opaque;
-    cco->ret = bdrv_co_check(cco->bs, cco->res, cco->fix);
-    aio_wait_kick();
-}
-
-int bdrv_check(BlockDriverState *bs,
-               BdrvCheckResult *res, BdrvCheckMode fix)
-{
-    Coroutine *co;
-    CheckCo cco = {
-        .bs = bs,
-        .res = res,
-        .ret = -EINPROGRESS,
-        .fix = fix,
-    };
-
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_check_co_entry(&cco);
-    } else {
-        co = qemu_coroutine_create(bdrv_check_co_entry, &cco);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, cco.ret == -EINPROGRESS);
-    }
-
-    return cco.ret;
-}
-
 /*
  * Return values:
  * 0        - success
@@ -5728,42 +5691,6 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
     return 0;
 }
 
-typedef struct InvalidateCacheCo {
-    BlockDriverState *bs;
-    Error **errp;
-    bool done;
-    int ret;
-} InvalidateCacheCo;
-
-static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
-{
-    InvalidateCacheCo *ico = opaque;
-    ico->ret = bdrv_co_invalidate_cache(ico->bs, ico->errp);
-    ico->done = true;
-    aio_wait_kick();
-}
-
-int bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
-{
-    Coroutine *co;
-    InvalidateCacheCo ico = {
-        .bs = bs,
-        .done = false,
-        .errp = errp
-    };
-
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_invalidate_cache_co_entry(&ico);
-    } else {
-        co = qemu_coroutine_create(bdrv_invalidate_cache_co_entry, &ico);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, !ico.done);
-    }
-
-    return ico.ret;
-}
-
 void bdrv_invalidate_cache_all(Error **errp)
 {
     BlockDriverState *bs;
diff --git a/block/io.c b/block/io.c
index 676c932caf..5270d68d72 100644
--- a/block/io.c
+++ b/block/io.c
@@ -890,50 +890,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
     return 0;
 }
 
-typedef int coroutine_fn BdrvRequestEntry(void *opaque);
-typedef struct BdrvRunCo {
-    BdrvRequestEntry *entry;
-    void *opaque;
-    int ret;
-    bool done;
-    Coroutine *co; /* Coroutine, running bdrv_run_co_entry, for debugging */
-} BdrvRunCo;
-
-static void coroutine_fn bdrv_run_co_entry(void *opaque)
-{
-    BdrvRunCo *arg = opaque;
-
-    arg->ret = arg->entry(arg->opaque);
-    arg->done = true;
-    aio_wait_kick();
-}
-
-static int bdrv_run_co(BlockDriverState *bs, BdrvRequestEntry *entry,
-                       void *opaque)
-{
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        return entry(opaque);
-    } else {
-        BdrvRunCo s = { .entry = entry, .opaque = opaque };
-
-        s.co = qemu_coroutine_create(bdrv_run_co_entry, &s);
-        bdrv_coroutine_enter(bs, s.co);
-
-        BDRV_POLL_WHILE(bs, !s.done);
-
-        return s.ret;
-    }
-}
-
-typedef struct RwCo {
-    BdrvChild *child;
-    int64_t offset;
-    QEMUIOVector *qiov;
-    bool is_write;
-    BdrvRequestFlags flags;
-} RwCo;
-
 int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
                               QEMUIOVector *qiov, bool is_write,
                               BdrvRequestFlags flags)
@@ -945,32 +901,6 @@ int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
     }
 }
 
-static int coroutine_fn bdrv_rw_co_entry(void *opaque)
-{
-    RwCo *rwco = opaque;
-
-    return bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
-                        rwco->is_write, rwco->flags);
-}
-
-/*
- * Process a vectored synchronous request using coroutines
- */
-int bdrv_prwv(BdrvChild *child, int64_t offset,
-              QEMUIOVector *qiov, bool is_write,
-              BdrvRequestFlags flags)
-{
-    RwCo rwco = {
-        .child = child,
-        .offset = offset,
-        .qiov = qiov,
-        .is_write = is_write,
-        .flags = flags,
-    };
-
-    return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco);
-}
-
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
@@ -2247,18 +2177,6 @@ int bdrv_flush_all(void)
     return result;
 }
 
-
-typedef struct BdrvCoBlockStatusData {
-    BlockDriverState *bs;
-    BlockDriverState *base;
-    bool want_zero;
-    int64_t offset;
-    int64_t bytes;
-    int64_t *pnum;
-    int64_t *map;
-    BlockDriverState **file;
-} BdrvCoBlockStatusData;
-
 int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
                                                 bool want_zero,
                                                 int64_t offset,
@@ -2510,43 +2428,6 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
     return ret;
 }
 
-/* Coroutine wrapper for bdrv_block_status_above() */
-static int coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
-{
-    BdrvCoBlockStatusData *data = opaque;
-
-    return bdrv_co_common_block_status_above(data->bs, data->base,
-                                             data->want_zero,
-                                             data->offset, data->bytes,
-                                             data->pnum, data->map, data->file);
-}
-
-/*
- * Synchronous wrapper around bdrv_co_block_status_above().
- *
- * See bdrv_co_block_status_above() for details.
- */
-int bdrv_common_block_status_above(BlockDriverState *bs,
-                                   BlockDriverState *base,
-                                   bool want_zero, int64_t offset,
-                                   int64_t bytes, int64_t *pnum,
-                                   int64_t *map,
-                                   BlockDriverState **file)
-{
-    BdrvCoBlockStatusData data = {
-        .bs = bs,
-        .base = base,
-        .want_zero = want_zero,
-        .offset = offset,
-        .bytes = bytes,
-        .pnum = pnum,
-        .map = map,
-        .file = file,
-    };
-
-    return bdrv_run_co(bs, bdrv_block_status_above_co_entry, &data);
-}
-
 int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum,
                             int64_t *map, BlockDriverState **file)
@@ -2640,13 +2521,6 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-typedef struct BdrvVmstateCo {
-    BlockDriverState    *bs;
-    QEMUIOVector        *qiov;
-    int64_t             pos;
-    bool                is_read;
-} BdrvVmstateCo;
-
 int coroutine_fn
 bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
                    bool is_read)
@@ -2672,26 +2546,6 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
     return ret;
 }
 
-static int coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
-{
-    BdrvVmstateCo *co = opaque;
-
-    return bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
-}
-
-int bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-                    bool is_read)
-{
-    BdrvVmstateCo data = {
-        .bs         = bs,
-        .qiov       = qiov,
-        .pos        = pos,
-        .is_read    = is_read,
-    };
-
-    return bdrv_run_co(bs, bdrv_co_rw_vmstate_entry, &data);
-}
-
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
@@ -2767,11 +2621,6 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb)
 /**************************************************************/
 /* Coroutine block device emulation */
 
-static int coroutine_fn bdrv_flush_co_entry(void *opaque)
-{
-    return bdrv_co_flush(opaque);
-}
-
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 {
     int current_gen;
@@ -2884,24 +2733,6 @@ early_exit:
     return ret;
 }
 
-int bdrv_flush(BlockDriverState *bs)
-{
-    return bdrv_run_co(bs, bdrv_flush_co_entry, bs);
-}
-
-typedef struct DiscardCo {
-    BdrvChild *child;
-    int64_t offset;
-    int64_t bytes;
-} DiscardCo;
-
-static int coroutine_fn bdrv_pdiscard_co_entry(void *opaque)
-{
-    DiscardCo *rwco = opaque;
-
-    return bdrv_co_pdiscard(rwco->child, rwco->offset, rwco->bytes);
-}
-
 int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
                                   int64_t bytes)
 {
@@ -3016,17 +2847,6 @@ out:
     return ret;
 }
 
-int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes)
-{
-    DiscardCo rwco = {
-        .child = child,
-        .offset = offset,
-        .bytes = bytes,
-    };
-
-    return bdrv_run_co(child->bs, bdrv_pdiscard_co_entry, &rwco);
-}
-
 int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
 {
     BlockDriver *drv = bs->drv;
@@ -3424,35 +3244,3 @@ out:
 
     return ret;
 }
-
-typedef struct TruncateCo {
-    BdrvChild *child;
-    int64_t offset;
-    bool exact;
-    PreallocMode prealloc;
-    BdrvRequestFlags flags;
-    Error **errp;
-} TruncateCo;
-
-static int coroutine_fn bdrv_truncate_co_entry(void *opaque)
-{
-    TruncateCo *tco = opaque;
-
-    return bdrv_co_truncate(tco->child, tco->offset, tco->exact,
-                            tco->prealloc, tco->flags, tco->errp);
-}
-
-int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-                  PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
-{
-    TruncateCo tco = {
-        .child      = child,
-        .offset     = offset,
-        .exact      = exact,
-        .prealloc   = prealloc,
-        .flags      = flags,
-        .errp       = errp,
-    };
-
-    return bdrv_run_co(child->bs, bdrv_truncate_co_entry, &tco);
-}
-- 
2.21.3

Re: [PATCH v8 5/7] block: generate coroutine-wrapper code

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use code generation implemented in previous commit to generated
> coroutine wrappers in block.c and block/io.c
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h    |   6 +-
>  include/block/block.h |  16 ++--
>  block.c               |  73 ---------------
>  block/io.c            | 212 ------------------------------------------
>  4 files changed, 13 insertions(+), 294 deletions(-)

Nice

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

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

[PATCH v8 6/7] block: drop bdrv_prwv

[Vladimir Sementsov-Ogievskiy]

Now that we are not maintaining boilerplate code for coroutine
wrappers, there is no more sense in keeping the extra indirection layer
of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()
and bdrv_pwritev().

Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
success, auto generated functions will instead return zero, as their
_co_ prototype. Still, it's simple to make the conversion safe: the
only external user of bdrv_pwritev() is test-bdrv-drain, and it is
comfortable enough with bdrv_co_pwritev() instead. So prototypes are
moved to local block/coroutines.h. Next, the only internal use is
bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
success.

Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()
to return 0 on success. But this requires audit (and probably
conversion) of all their users, let's leave it for another day
refactoring.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/coroutines.h      | 10 ++++-----
 include/block/block.h   |  2 --
 block/io.c              | 49 ++++++++---------------------------------
 tests/test-bdrv-drain.c |  2 +-
 4 files changed, 15 insertions(+), 48 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h
index c62b3a2697..6c63a819c9 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -31,12 +31,12 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs,
                                BdrvCheckResult *res, BdrvCheckMode fix);
 int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
 
-int coroutine_fn
-bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
-             bool is_write, BdrvRequestFlags flags);
 int generated_co_wrapper
-bdrv_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
-          bool is_write, BdrvRequestFlags flags);
+bdrv_preadv(BdrvChild *child, int64_t offset, unsigned int bytes,
+            QEMUIOVector *qiov, BdrvRequestFlags flags);
+int generated_co_wrapper
+bdrv_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes,
+             QEMUIOVector *qiov, BdrvRequestFlags flags);
 
 int coroutine_fn
 bdrv_co_common_block_status_above(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index d8fb02fa2a..b8b4c177de 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,9 +383,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags);
 int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes);
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes);
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov);
 int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
                      const void *buf, int count);
 /*
diff --git a/block/io.c b/block/io.c
index 5270d68d72..68d7d9cf80 100644
--- a/block/io.c
+++ b/block/io.c
@@ -890,23 +890,11 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
     return 0;
 }
 
-int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
-                              QEMUIOVector *qiov, bool is_write,
-                              BdrvRequestFlags flags)
-{
-    if (is_write) {
-        return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
-    } else {
-        return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
-    }
-}
-
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
-    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
-
-    return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
+    return bdrv_pwritev(child, offset, bytes, NULL,
+                        BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
@@ -950,41 +938,19 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
     }
 }
 
-/* return < 0 if error. See bdrv_pwrite() for the return codes */
-int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
-    int ret;
-
-    ret = bdrv_prwv(child, offset, qiov, false, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return qiov->size;
-}
-
 /* See bdrv_pwrite() for the return codes */
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
 {
+    int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
-    return bdrv_preadv(child, offset, &qiov);
-}
-
-int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
-{
-    int ret;
+    ret = bdrv_preadv(child, offset, bytes, &qiov,  0);
 
-    ret = bdrv_prwv(child, offset, qiov, true, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return qiov->size;
+    return ret < 0 ? ret : bytes;
 }
 
 /* Return no. of bytes on success or < 0 on error. Important errors are:
@@ -995,13 +961,16 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 */
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
 {
+    int ret;
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
-    return bdrv_pwritev(child, offset, &qiov);
+    ret = bdrv_pwritev(child, offset, bytes, &qiov, 0);
+
+    return ret < 0 ? ret : bytes;
 }
 
 /*
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 1107271840..1595bbc92e 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1872,7 +1872,7 @@ static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,
         }
         s->io_co = NULL;
 
-        ret = bdrv_preadv(bs->backing, offset, qiov);
+        ret = bdrv_co_preadv(bs->backing, offset, bytes, qiov, 0);
         s->has_read = true;
 
         /* Wake up drain_co if it runs */
-- 
2.21.3

Re: [PATCH v8 6/7] block: drop bdrv_prwv

[Philippe Mathieu-Daudé]

On 9/15/20 6:44 PM, Vladimir Sementsov-Ogievskiy wrote:
> Now that we are not maintaining boilerplate code for coroutine
> wrappers, there is no more sense in keeping the extra indirection layer
> of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()
> and bdrv_pwritev().
> 
> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
> success, auto generated functions will instead return zero, as their
> _co_ prototype. Still, it's simple to make the conversion safe: the
> only external user of bdrv_pwritev() is test-bdrv-drain, and it is
> comfortable enough with bdrv_co_pwritev() instead. So prototypes are
> moved to local block/coroutines.h. Next, the only internal use is
> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
> success.
> 
> Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()
> to return 0 on success. But this requires audit (and probably
> conversion) of all their users, let's leave it for another day
> refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h      | 10 ++++-----
>  include/block/block.h   |  2 --
>  block/io.c              | 49 ++++++++---------------------------------
>  tests/test-bdrv-drain.c |  2 +-
>  4 files changed, 15 insertions(+), 48 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v8 6/7] block: drop bdrv_prwv

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:10PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Now that we are not maintaining boilerplate code for coroutine
> wrappers, there is no more sense in keeping the extra indirection layer
> of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv()
> and bdrv_pwritev().
> 
> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
> success, auto generated functions will instead return zero, as their
> _co_ prototype. Still, it's simple to make the conversion safe: the
> only external user of bdrv_pwritev() is test-bdrv-drain, and it is
> comfortable enough with bdrv_co_pwritev() instead. So prototypes are
> moved to local block/coroutines.h. Next, the only internal use is
> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
> success.
> 
> Of course, it would be great to convert bdrv_pread() and bdrv_pwrite()
> to return 0 on success. But this requires audit (and probably
> conversion) of all their users, let's leave it for another day
> refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h      | 10 ++++-----
>  include/block/block.h   |  2 --
>  block/io.c              | 49 ++++++++---------------------------------
>  tests/test-bdrv-drain.c |  2 +-
>  4 files changed, 15 insertions(+), 48 deletions(-)

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

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

[PATCH v8 7/7] block/io: refactor save/load vmstate

[Vladimir Sementsov-Ogievskiy]

Like for read/write in a previous commit, drop extra indirection layer,
generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/coroutines.h    | 10 +++----
 include/block/block.h |  6 ++--
 block/io.c            | 67 ++++++++++++++++++++++---------------------
 3 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h
index 6c63a819c9..f69179f5ef 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -57,11 +57,9 @@ bdrv_common_block_status_above(BlockDriverState *bs,
                                int64_t *map,
                                BlockDriverState **file);
 
-int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-                   bool is_read);
-int generated_co_wrapper
-bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-                bool is_read);
+int coroutine_fn bdrv_co_readv_vmstate(BlockDriverState *bs,
+                                       QEMUIOVector *qiov, int64_t pos);
+int coroutine_fn bdrv_co_writev_vmstate(BlockDriverState *bs,
+                                        QEMUIOVector *qiov, int64_t pos);
 
 #endif /* BLOCK_COROUTINES_INT_H */
diff --git a/include/block/block.h b/include/block/block.h
index b8b4c177de..6cd789724b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -572,8 +572,10 @@ int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
 char *path_combine(const char *base_path, const char *filename);
 
-int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
+int generated_co_wrapper
+bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
+int generated_co_wrapper
+bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size);
 
diff --git a/block/io.c b/block/io.c
index 68d7d9cf80..84f82bc069 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2491,66 +2491,67 @@ int bdrv_is_allocated_above(BlockDriverState *top,
 }
 
 int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-                   bool is_read)
+bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 {
     BlockDriver *drv = bs->drv;
     int ret = -ENOTSUP;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     bdrv_inc_in_flight(bs);
 
-    if (!drv) {
-        ret = -ENOMEDIUM;
-    } else if (drv->bdrv_load_vmstate) {
-        if (is_read) {
-            ret = drv->bdrv_load_vmstate(bs, qiov, pos);
-        } else {
-            ret = drv->bdrv_save_vmstate(bs, qiov, pos);
-        }
+    if (drv->bdrv_load_vmstate) {
+        ret = drv->bdrv_load_vmstate(bs, qiov, pos);
     } else if (bs->file) {
-        ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
+        ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
     }
 
     bdrv_dec_in_flight(bs);
+
     return ret;
 }
 
-int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
-                      int64_t pos, int size)
+int coroutine_fn
+bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 {
-    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
-    int ret;
+    BlockDriver *drv = bs->drv;
+    int ret = -ENOTSUP;
 
-    ret = bdrv_writev_vmstate(bs, &qiov, pos);
-    if (ret < 0) {
-        return ret;
+    if (!drv) {
+        return -ENOMEDIUM;
     }
 
-    return size;
-}
+    bdrv_inc_in_flight(bs);
 
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
-{
-    return bdrv_rw_vmstate(bs, qiov, pos, false);
+    if (drv->bdrv_load_vmstate) {
+        ret = drv->bdrv_save_vmstate(bs, qiov, pos);
+    } else if (bs->file) {
+        ret = bdrv_co_writev_vmstate(bs->file->bs, qiov, pos);
+    }
+
+    bdrv_dec_in_flight(bs);
+
+    return ret;
 }
 
-int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
+int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
-    int ret;
-
-    ret = bdrv_readv_vmstate(bs, &qiov, pos);
-    if (ret < 0) {
-        return ret;
-    }
+    int ret = bdrv_writev_vmstate(bs, &qiov, pos);
 
-    return size;
+    return ret < 0 ? ret : size;
 }
 
-int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
+int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
+                      int64_t pos, int size)
 {
-    return bdrv_rw_vmstate(bs, qiov, pos, true);
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
+    int ret = bdrv_readv_vmstate(bs, &qiov, pos);
+
+    return ret < 0 ? ret : size;
 }
 
 /**************************************************************/
-- 
2.21.3

Re: [PATCH v8 7/7] block/io: refactor save/load vmstate

[Eric Blake]

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Like for read/write in a previous commit, drop extra indirection layer,
> generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/coroutines.h    | 10 +++----
>   include/block/block.h |  6 ++--
>   block/io.c            | 67 ++++++++++++++++++++++---------------------
>   3 files changed, 42 insertions(+), 41 deletions(-)
> 
> diff --git a/block/coroutines.h b/block/coroutines.h

>   int coroutine_fn
> -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
> -                   bool is_read)
> +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>   {
>       BlockDriver *drv = bs->drv;
>       int ret = -ENOTSUP;
>   
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +
>       bdrv_inc_in_flight(bs);
>   
> -    if (!drv) {
> -        ret = -ENOMEDIUM;
> -    } else if (drv->bdrv_load_vmstate) {
> -        if (is_read) {
> -            ret = drv->bdrv_load_vmstate(bs, qiov, pos);
> -        } else {
> -            ret = drv->bdrv_save_vmstate(bs, qiov, pos);
> -        }
> +    if (drv->bdrv_load_vmstate) {
> +        ret = drv->bdrv_load_vmstate(bs, qiov, pos);

This one makes sense;

>       } else if (bs->file) {
> -        ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
> +        ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
>       }
>   
>       bdrv_dec_in_flight(bs);
> +
>       return ret;
>   }
>   
> -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
> -                      int64_t pos, int size)
> +int coroutine_fn
> +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>   {
> -    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
> -    int ret;
> +    BlockDriver *drv = bs->drv;
> +    int ret = -ENOTSUP;
>   
> -    ret = bdrv_writev_vmstate(bs, &qiov, pos);
> -    if (ret < 0) {
> -        return ret;
> +    if (!drv) {
> +        return -ENOMEDIUM;
>       }
>   
> -    return size;
> -}
> +    bdrv_inc_in_flight(bs);
>   
> -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> -{
> -    return bdrv_rw_vmstate(bs, qiov, pos, false);
> +    if (drv->bdrv_load_vmstate) {
> +        ret = drv->bdrv_save_vmstate(bs, qiov, pos);

but this one looks awkward. It represents the pre-patch logic, but it 
would be nicer to check for bdrv_save_vmstate.  With that tweak, my R-b 
still stands.

I had an interesting time applying this patch due to merge conflicts 
with the new bdrv_primary_bs() changes that landed in the meantime.

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

Re: [PATCH v8 7/7] block/io: refactor save/load vmstate

[Vladimir Sementsov-Ogievskiy]

23.09.2020 23:10, Eric Blake wrote:
> On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Like for read/write in a previous commit, drop extra indirection layer,
>> generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/coroutines.h    | 10 +++----
>>   include/block/block.h |  6 ++--
>>   block/io.c            | 67 ++++++++++++++++++++++---------------------
>>   3 files changed, 42 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/coroutines.h b/block/coroutines.h
> 
>>   int coroutine_fn
>> -bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
>> -                   bool is_read)
>> +bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret = -ENOTSUP;
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +
>>       bdrv_inc_in_flight(bs);
>> -    if (!drv) {
>> -        ret = -ENOMEDIUM;
>> -    } else if (drv->bdrv_load_vmstate) {
>> -        if (is_read) {
>> -            ret = drv->bdrv_load_vmstate(bs, qiov, pos);
>> -        } else {
>> -            ret = drv->bdrv_save_vmstate(bs, qiov, pos);
>> -        }
>> +    if (drv->bdrv_load_vmstate) {
>> +        ret = drv->bdrv_load_vmstate(bs, qiov, pos);
> 
> This one makes sense;
> 
>>       } else if (bs->file) {
>> -        ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
>> +        ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
>>       }
>>       bdrv_dec_in_flight(bs);
>> +
>>       return ret;
>>   }
>> -int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
>> -                      int64_t pos, int size)
>> +int coroutine_fn
>> +bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>>   {
>> -    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
>> -    int ret;
>> +    BlockDriver *drv = bs->drv;
>> +    int ret = -ENOTSUP;
>> -    ret = bdrv_writev_vmstate(bs, &qiov, pos);
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>>       }
>> -    return size;
>> -}
>> +    bdrv_inc_in_flight(bs);
>> -int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>> -{
>> -    return bdrv_rw_vmstate(bs, qiov, pos, false);
>> +    if (drv->bdrv_load_vmstate) {
>> +        ret = drv->bdrv_save_vmstate(bs, qiov, pos);
> 
> but this one looks awkward. It represents the pre-patch logic, but it would be nicer to check for bdrv_save_vmstate.  With that tweak, my R-b still stands.

Agree.

> 
> I had an interesting time applying this patch due to merge conflicts with the new bdrv_primary_bs() changes that landed in the meantime.
> 

Thanks a lot!

To clarify: did you finally staged the series to send a pull request? Or Stefan should do it? Should I make a v9?

-- 
Best regards,
Vladimir

Re: [PATCH v8 7/7] block/io: refactor save/load vmstate

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:11PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Like for read/write in a previous commit, drop extra indirection layer,
> generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h    | 10 +++----
>  include/block/block.h |  6 ++--
>  block/io.c            | 67 ++++++++++++++++++++++---------------------
>  3 files changed, 42 insertions(+), 41 deletions(-)

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

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

Re: [PATCH v8 0/7] coroutines: generate wrapper code

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 07:44:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> The aim of the series is to reduce code-duplication and writing
> parameters structure-packing by hand around coroutine function wrappers.
> 
> Benefits:
>  - no code duplication
>  - less indirection
> 
> v8:
> 04: - rebase on meson build
>         - script interface is changed to satisfy meson custom_target
>     - rename script s/coroutine-wrapper.py/block-coroutine-wrapper.py/
>     - add docs/devel/block-coroutine-wrapper.rst
> 
> Vladimir Sementsov-Ogievskiy (7):
>   block: return error-code from bdrv_invalidate_cache
>   block/io: refactor coroutine wrappers
>   block: declare some coroutine functions in block/coroutines.h
>   scripts: add block-coroutine-wrapper.py
>   block: generate coroutine-wrapper code
>   block: drop bdrv_prwv
>   block/io: refactor save/load vmstate
> 
>  docs/devel/block-coroutine-wrapper.rst |  54 ++++
>  block/block-gen.h                      |  49 ++++
>  block/coroutines.h                     |  65 +++++
>  include/block/block.h                  |  34 ++-
>  block.c                                |  97 ++-----
>  block/io.c                             | 336 ++++---------------------
>  tests/test-bdrv-drain.c                |   2 +-
>  block/meson.build                      |   8 +
>  scripts/block-coroutine-wrapper.py     | 187 ++++++++++++++
>  9 files changed, 451 insertions(+), 381 deletions(-)
>  create mode 100644 docs/devel/block-coroutine-wrapper.rst
>  create mode 100644 block/block-gen.h
>  create mode 100644 block/coroutines.h
>  create mode 100755 scripts/block-coroutine-wrapper.py

Please send a v9 and I'll merge it.

Stefan

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