diff mbox series

[RFC,v2] coroutines: generate wrapper code

Message ID 20190220100358.25566-1-vsementsov@virtuozzo.com
State New
Headers show
Series [RFC,v2] coroutines: generate wrapper code | expand

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 20, 2019, 10:03 a.m. UTC 
Hi all!

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, use separate bool for void functions
  - fill the struct and create coroutine from _entry function and this
    struct as a parameter

Here is a template code + example how it can be used to drop a lot of
similar code.

TODO: make coroutine-wrapper template file to be header itself, or
      generate header from it

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

v2: - don't generate poll-loop wrappers
    - drop jinja

 Makefile                     |  5 ++
 Makefile.objs                |  2 +-
 include/block/block.h        |  3 ++
 include/block/block_int.h    |  8 +++
 block.c                      | 76 +++++++---------------------
 coroutine-wrapper            |  2 +
 scripts/coroutine-wrapper.py | 98 ++++++++++++++++++++++++++++++++++++
 7 files changed, 136 insertions(+), 58 deletions(-)
 create mode 100644 coroutine-wrapper
 create mode 100755 scripts/coroutine-wrapper.py

Comments

Stefan Hajnoczi Feb. 21, 2019, 10:53 a.m. UTC | #1 
On Wed, Feb 20, 2019 at 01:03:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> 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, use separate bool for void functions
>   - fill the struct and create coroutine from _entry function and this
>     struct as a parameter
> 
> Here is a template code + example how it can be used to drop a lot of
> similar code.
> 
> TODO: make coroutine-wrapper template file to be header itself, or
>       generate header from it

Yes, please.  For example, block/coroutines.h:

  #include "block/block.h"

  int coroutine_fn bdrv_co_check(BlockDriverState *bs,
                                 BdrvCheckResult *res, BdrvCheckMode fix);
  void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);

Now include/block/block.h doesn't need to be modified and these
coroutine function prototypes stay private to the block layer (they are
not in include/).

The code generator script can parse coroutines.h to generate
__create_co() functions.  This also makes the tool independent of the
block layer - the output C file just needs to include the input header
file:

 /*
  * File is generated by scripts/coroutine-wrapper.py
  */

 #include "qemu/osdep.h"
 #include "block/coroutines.h" <--- from coroutine-wrapper command-line

 ...
no-reply@patchew.org March 8, 2019, 4:03 a.m. UTC | #2 
Patchew URL: https://patchew.org/QEMU/20190220100358.25566-1-vsementsov@virtuozzo.com/



Hi,

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

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




The full log is available at
http://patchew.org/logs/20190220100358.25566-1-vsementsov@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 1278a3eb52..8d19b06cf1 100644
--- a/Makefile
+++ b/Makefile
@@ -122,6 +122,8 @@  endif
 
 GENERATED_FILES += module_block.h
 
+GENERATED_FILES += block-gen.c
+
 TRACE_HEADERS = trace-root.h $(trace-events-subdirs:%=%/trace.h)
 TRACE_SOURCES = trace-root.c $(trace-events-subdirs:%=%/trace.c)
 TRACE_DTRACE =
@@ -138,6 +140,9 @@  GENERATED_FILES += $(TRACE_SOURCES)
 GENERATED_FILES += $(BUILD_DIR)/trace-events-all
 GENERATED_FILES += .git-submodule-status
 
+block-gen.c: coroutine-wrapper $(SRC_PATH)/scripts/coroutine-wrapper.py
+	$(call quiet-command, python $(SRC_PATH)/scripts/coroutine-wrapper.py < $< > $@,"GEN","$(TARGET_DIR)$@")
+
 trace-group-name = $(shell dirname $1 | sed -e 's/[^a-zA-Z0-9]/_/g')
 
 tracetool-y = $(SRC_PATH)/scripts/tracetool.py
diff --git a/Makefile.objs b/Makefile.objs
index 67a054b08a..16159d3e0f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -23,7 +23,7 @@  slirp-obj-$(CONFIG_SLIRP) = slirp/
 # block-obj-y is code used by both qemu system emulation and qemu-img
 
 block-obj-y += nbd/
-block-obj-y += block.o blockjob.o job.o
+block-obj-y += block.o blockjob.o job.o block-gen.o
 block-obj-y += block/ scsi/
 block-obj-y += qemu-io-cmds.o
 block-obj-$(CONFIG_REPLICATION) += replication.o
diff --git a/include/block/block.h b/include/block/block.h
index 57233cf2c0..61b2ca6fe5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -371,6 +371,8 @@  typedef enum {
 } BdrvCheckMode;
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
+int coroutine_fn bdrv_co_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
@@ -399,6 +401,7 @@  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);
+void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
 void bdrv_invalidate_cache_all(Error **errp);
 int bdrv_inactivate_all(void);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216..9af6507be7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1175,4 +1175,12 @@  int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
 
 int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
 
+
+Coroutine *bdrv_co_invalidate_cache__create_co(bool *_in_progress,
+                                               BlockDriverState *bs,
+                                               Error **errp);
+Coroutine *bdrv_co_check__create_co(bool *_in_progress, int *_ret,
+                                    BlockDriverState *bs, BdrvCheckResult *res,
+                                    BdrvCheckMode fix);
+
 #endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index b67d9b7b65..94999bc0fc 100644
--- a/block.c
+++ b/block.c
@@ -3706,8 +3706,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;
@@ -3720,44 +3720,25 @@  static 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 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)
 {
+    int ret;
+    bool in_progress;
     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 bdrv_co_check(bs, res, fix);
     }
 
-    return cco.ret;
-}
+    co = bdrv_co_check__create_co(&in_progress, &ret, bs, res, fix);
+    bdrv_coroutine_enter(bs, co);
+    BDRV_POLL_WHILE(bs, in_progress);
 
+    return ret;
+}
 /*
+ *
  * Return values:
  * 0        - success
  * -EINVAL  - backing format specified, but no file
@@ -4623,8 +4604,7 @@  void bdrv_init_with_whitelist(void)
     bdrv_init();
 }
 
-static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
-                                                  Error **errp)
+void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BdrvChild *child, *parent;
     uint64_t perm, shared_perm;
@@ -4705,37 +4685,19 @@  static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
     }
 }
 
-typedef struct InvalidateCacheCo {
-    BlockDriverState *bs;
-    Error **errp;
-    bool done;
-} InvalidateCacheCo;
-
-static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque)
-{
-    InvalidateCacheCo *ico = opaque;
-    bdrv_co_invalidate_cache(ico->bs, ico->errp);
-    ico->done = true;
-    aio_wait_kick();
-}
-
 void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
+    bool in_progress;
     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);
+        bdrv_co_invalidate_cache(bs, errp);
+        return;
     }
+
+    co = bdrv_co_invalidate_cache__create_co(&in_progress, bs, errp);
+    bdrv_coroutine_enter(bs, co);
+    BDRV_POLL_WHILE(bs, in_progress);
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
diff --git a/coroutine-wrapper b/coroutine-wrapper
new file mode 100644
index 0000000000..47bcd2e6a0
--- /dev/null
+++ b/coroutine-wrapper
@@ -0,0 +1,2 @@ 
+int bdrv_co_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
+void bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
diff --git a/scripts/coroutine-wrapper.py b/scripts/coroutine-wrapper.py
new file mode 100755
index 0000000000..35b0ba8c59
--- /dev/null
+++ b/scripts/coroutine-wrapper.py
@@ -0,0 +1,98 @@ 
+#!/usr/bin/env python
+
+import re
+
+header = """/*
+ * File is generated by scripts/coroutine-wrapper.py
+ */
+
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+"""
+
+template = """
+/*
+ * Wrappers for $name$
+ */
+
+typedef struct $name$__ArgumentsPack {
+    $fields$
+} $name$__ArgumentsPack;
+
+static void $name$__entry(void *opaque)
+{
+    $name$__ArgumentsPack *pack = opaque;
+
+    $call$;
+
+    *pack->_in_progress = false;
+    aio_wait_kick();
+    g_free(opaque);
+}
+
+Coroutine *$name$__create_co($args_def$)
+{
+    $name$__ArgumentsPack *pack = g_new($name$__ArgumentsPack, 1);
+
+    *pack = ($name$__ArgumentsPack) {
+        $initializers$
+    };
+
+    *_in_progress = true;
+
+    return qemu_coroutine_create($name$__entry, pack);
+}
+"""
+
+# We want to use python string.format() formatter, which uses curly brackets
+# as separators. But it's not comfortable with C. So, we used dollars instead,
+# and now is the time to escape curly brackets and convert dollars.
+template = template.replace('{', '{{').replace('}', '}}')
+template = re.sub(r'\$(\w+)\$', r'{\1}', template)
+
+func_decl_re = re.compile(r'^([^(]+) ([a-z][a-z0-9_]*)\((.*)\)$')
+param_re = re.compile(r'(?P<def>.*[ *](?P<name>[a-z][a-z0-9_]*))')
+
+
+def format_args(args, format, separator):
+    return separator.join(format.format(**arg) for arg in args)
+
+
+def make_wrapper(function_declaration):
+    try:
+        m = func_decl_re.match(function_declaration)
+        ret_type = m.group(1).strip()
+        name = m.group(2)
+        raw_args = m.group(3).split(', ')
+        args = [param_re.match(arg).groupdict() for arg in raw_args]
+    except AttributeError:
+        raise ValueError('Failed to parse function declaration')
+
+    xargs = [{'def': 'bool *_in_progress', 'name': '_in_progress'}] + args
+
+    has_ret = ret_type != 'void'
+    if has_ret:
+        xargs.insert(1, {'def': ret_type + ' * _ret', 'name': '_ret'})
+
+    params = {
+        'name': name,
+        'args_def': format_args(xargs, '{def}', ', '),
+        'fields': format_args(xargs, '{def};', '\n    '),
+        'initializers': format_args(xargs, '.{name} = {name},', '\n        '),
+        'call': '{}{}({})'.format('*pack->_ret = ' if has_ret else '',
+                                  name,
+                                  format_args(args, 'pack->{name}', ', '))
+    }
+
+    return template.format(**params)
+
+
+if __name__ == '__main__':
+    import sys
+
+    print(header)
+    try:
+        for ind, line in enumerate(sys.stdin):
+            print(make_wrapper(line))
+    except ValueError as e:
+        sys.exit(('ERROR: {} at line {}:\n{}').format(e, ind + 1, line))