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

[Eric Blake]

On 6/9/20 9:13 AM, 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, use separate bool for void functions

Stale comment, now that you got rid of void functions earlier in the series.

>    - 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/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>
> ---
>   Makefile                             |   8 ++
>   block/block-gen.h                    |  55 +++++++++
>   include/block/generated-co-wrapper.h |  35 ++++++
>   block/Makefile.objs                  |   1 +
>   scripts/coroutine-wrapper.py         | 174 +++++++++++++++++++++++++++
>   5 files changed, 273 insertions(+)
>   create mode 100644 block/block-gen.h
>   create mode 100644 include/block/generated-co-wrapper.h
>   create mode 100755 scripts/coroutine-wrapper.py

My python review is weak, but here goes.

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

> +
> +#include "block/block_int.h"
> +
> +/* This function is called at the end of generated coroutine entries. */
> +static inline void bdrv_poll_co__on_exit(void)
> +{
> +    aio_wait_kick();
> +}

I still think it's worth inlining aio_wait_kick() into the generated 
code, instead of this one-line wrapper function.  Patch below.

> +
> +/* 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/generated-co-wrapper.h 
> b/include/block/generated-co-wrapper.h
> new file mode 100644
> index 0000000000..62c6e053ba
> --- /dev/null
> +++ b/include/block/generated-co-wrapper.h

> +#ifndef BLOCK_GENERATED_CO_WRAPPER_H
> +#define BLOCK_GENERATED_CO_WRAPPER_H
> +
> +/*
> + * generated_co_wrapper
> + * Function specifier, which does nothing but marking functions to be
> + * generated by scripts/coroutine-wrapper.py
> + */
> +#define generated_co_wrapper

Do we need a standalone header just for this definition, or could we 
stick it in include/block/block.h?  Also in my patch below.

> diff --git a/scripts/coroutine-wrapper.py b/scripts/coroutine-wrapper.py
> new file mode 100755
> index 0000000000..dbe6fb97d9
> --- /dev/null
> +++ b/scripts/coroutine-wrapper.py
> @@ -0,0 +1,174 @@
> +#!/usr/bin/env python3

> +"""Generate coroutine wrappers for block subsystem.
> +def prettify(code: str) -> str:
> +    """Prettify code using clang-format if available"""
> +
> +    try:
> +        style = '{IndentWidth: 4, BraceWrapping: {AfterFunction: true}, ' \
> +            'BreakBeforeBraces: Custom, SortIncludes: false, ' \
> +            'MaxEmptyLinesToKeep: 2}'

It looks odd to pass in style as a string (requiring \-newline) rather 
than a dict (which would not), but I guess that's because...

> +        p = subprocess.run(['clang-format', f'-style={style}'], check=True,

...you have to stringify it anyway for the subprocess command line.

> +class ParamDecl:
> +    param_re = re.compile(r'(?P<decl>'
> +                          r'(?P<type>.*[ *])'
> +                          r'(?P<name>[a-z][a-z0-9_]*)'

I guess you're safe not including A-Z based on our coding style.

> +                          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)
> +

Makes sense (requires coroutines to return int, but you fixed that 
earlier in the series).

> +
> +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:
...

Nothing else jumped out at me, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

Still, here's the promised diffs I'm thinking of:

for 4/7

From 8c089d488ed8d9778fd5ee1f18dbc3845e4349f2 Mon Sep 17 00:00:00 2001
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Date: Tue, 9 Jun 2020 17:13:26 +0300
Subject: [PATCH] fixup? scripts: add coroutine-wrapper.py

Worth squashing in to the coroutine generator?

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/coroutine-wrapper.py         |  5 ++--
 block/block-gen.h                    |  6 -----
 include/block/block.h                |  7 ++++++
 include/block/generated-co-wrapper.h | 35 ----------------------------
 4 files changed, 10 insertions(+), 43 deletions(-)
 delete mode 100644 include/block/generated-co-wrapper.h

diff --git a/scripts/coroutine-wrapper.py b/scripts/coroutine-wrapper.py
index dbe6fb97d9bd..0c2cf13401ce 100755
--- a/scripts/coroutine-wrapper.py
+++ b/scripts/coroutine-wrapper.py
@@ -57,7 +57,8 @@ def gen_header():

 #include "qemu/osdep.h"
 #include "block/coroutines.h"
-#include "block/block-gen.h"\
+#include "block/block-gen.h"
+#include "block/block_int.h"\
 """


@@ -139,7 +140,7 @@ static void coroutine_fn {name}_entry(void *opaque)
     s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
     s->poll_state.in_progress = false;

-    bdrv_poll_co__on_exit();
+    aio_wait_kick();
 }}

 int {func.name}({ func.gen_list('{decl}') })
diff --git a/block/block-gen.h b/block/block-gen.h
index 482d06737d10..f80cf4897d11 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -28,12 +28,6 @@

 #include "block/block_int.h"

-/* This function is called at the end of generated coroutine entries. */
-static inline void bdrv_poll_co__on_exit(void)
-{
-    aio_wait_kick();
-}
-
 /* Base structure for argument packing structures */
 typedef struct BdrvPollCo {
     BlockDriverState *bs;
diff --git a/include/block/block.h b/include/block/block.h
index 46965a77801c..59a002e06f23 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -10,6 +10,13 @@
 #include "block/blockjob.h"
 #include "qemu/hbitmap.h"

+/*
+ * generated_co_wrapper
+ * Function specifier, which does nothing but marking functions to be
+ * generated by scripts/coroutine-wrapper.py
+ */
+#define generated_co_wrapper
+
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 typedef struct BdrvChild BdrvChild;
diff --git a/include/block/generated-co-wrapper.h 
b/include/block/generated-co-wrapper.h
deleted file mode 100644
index 62c6e053ba12..000000000000
--- a/include/block/generated-co-wrapper.h
+++ /dev/null
@@ -1,35 +0,0 @@
-/*
- * Block layer I/O functions
- *
- * 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_GENERATED_CO_WRAPPER_H
-#define BLOCK_GENERATED_CO_WRAPPER_H
-
-/*
- * generated_co_wrapper
- * Function specifier, which does nothing but marking functions to be
- * generated by scripts/coroutine-wrapper.py
- */
-#define generated_co_wrapper
-
-#endif /* BLOCK_GENERATED_CO_WRAPPER_H */
--
2.27.0



for 5/7

diff --git i/block/coroutines.h w/block/coroutines.h
index 145a2d264524..c62b3a2697ca 100644
--- i/block/coroutines.h
+++ w/block/coroutines.h
@@ -26,7 +26,6 @@
 #define BLOCK_COROUTINES_INT_H

 #include "block/block_int.h"
-#include "block/generated-co-wrapper.h"

 int coroutine_fn bdrv_co_check(BlockDriverState *bs,
                                BdrvCheckResult *res, BdrvCheckMode fix);
diff --git i/include/block/block.h w/include/block/block.h
index 321d75675768..9f94c5905788 100644
--- i/include/block/block.h
+++ w/include/block/block.h
@@ -9,7 +9,6 @@
 #include "block/dirty-bitmap.h"
 #include "block/blockjob.h"
 #include "qemu/hbitmap.h"
-#include "block/generated-co-wrapper.h"

 /*
  * generated_co_wrapper


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