Re: [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Coroutine commands have to be declared as coroutine_fn, but the
> marker does not show up in the qapi-comands-* headers; likewise, the
> marshaling function calls the command and therefore must be coroutine_fn.
> Static analysis would want coroutine_fn to match between prototype and
> declaration, because in principle coroutines might be compiled to a
> completely different calling convention.  So we would like to add the
> marker to the header.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This continues (completes?) Kevin's commit 04f22362f14 (qapi: Add a
'coroutine' flag for commands).  I'm cc'ing him so he's aware; not a
request for review.

> ---
>  scripts/qapi/commands.py | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 79c5e5c3a989..a079378d1b8d 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -41,11 +41,13 @@
>  def gen_command_decl(name: str,
>                       arg_type: Optional[QAPISchemaObjectType],
>                       boxed: bool,
> -                     ret_type: Optional[QAPISchemaType]) -> str:
> +                     ret_type: Optional[QAPISchemaType],
> +                     coroutine: bool) -> str:
>      return mcgen('''
> -%(c_type)s qmp_%(c_name)s(%(params)s);
> +%(c_type)s %(coroutine_fn)sqmp_%(c_name)s(%(params)s);
>  ''',
>                   c_type=(ret_type and ret_type.c_type()) or 'void',
> +                 coroutine_fn='coroutine_fn ' if coroutine else '',
>                   c_name=c_name(name),
>                   params=build_params(arg_type, boxed, 'Error **errp'))
>  
> @@ -157,16 +159,21 @@ def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>                   c_type=ret_type.c_type(), c_name=ret_type.c_name())
>  
>  
> -def build_marshal_proto(name: str) -> str:
> -    return ('void qmp_marshal_%s(QDict *args, QObject **ret, Error **errp)'
> -            % c_name(name))
> +def build_marshal_proto(name: str,
> +                        coroutine: bool) -> str:

No need for the line break.  No big deal, and certainly not worth a
respin.

> +    return ('void %(coroutine_fn)sqmp_marshal_%(c_name)s(%(params)s)' % {
> +        'coroutine_fn': 'coroutine_fn ' if coroutine else '',
> +        'c_name': c_name(name),
> +        'params': 'QDict *args, QObject **ret, Error **errp',
> +    })
>  
>  
> -def gen_marshal_decl(name: str) -> str:
> +def gen_marshal_decl(name: str,
> +                     coroutine: bool) -> str:

Likewise.

>      return mcgen('''
>  %(proto)s;
>  ''',
> -                 proto=build_marshal_proto(name))
> +                 proto=build_marshal_proto(name, coroutine))
>  
>  
>  def gen_trace(name: str) -> str:
> @@ -181,7 +188,8 @@ def gen_marshal(name: str,
>                  arg_type: Optional[QAPISchemaObjectType],
>                  boxed: bool,
>                  ret_type: Optional[QAPISchemaType],
> -                gen_tracing: bool) -> str:
> +                gen_tracing: bool,
> +                coroutine: bool) -> str:
>      have_args = boxed or (arg_type and not arg_type.is_empty())
>      if have_args:
>          assert arg_type is not None
> @@ -195,7 +203,7 @@ def gen_marshal(name: str,
>      bool ok = false;
>      Visitor *v;
>  ''',
> -                proto=build_marshal_proto(name))
> +                proto=build_marshal_proto(name, coroutine))
>  
>      if ret_type:
>          ret += mcgen('''
> @@ -387,10 +395,11 @@ def visit_command(self,
>                             self._genh, self._genc):
>                  self._genc.add(gen_marshal_output(ret_type))
>          with ifcontext(ifcond, self._genh, self._genc):
> -            self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> -            self._genh.add(gen_marshal_decl(name))
> +            self._genh.add(gen_command_decl(name, arg_type, boxed,
> +                                            ret_type, coroutine))
> +            self._genh.add(gen_marshal_decl(name, coroutine))
>              self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> -                                       self._gen_tracing))
> +                                       self._gen_tracing, coroutine))
>              if self._gen_tracing:
>                  self._gen_trace_events.add(gen_trace(name))
>          with self._temp_module('./init'):

Since we have just three commands marked 'coroutine' so far
(block_resize, screendump, and one in tests), reviewing how the
generated code changes is quite practical.  I append the diff.
Highlighting the changes confirms that we add coroutine_fn to the
prototypes and the definitions of these three commands and marshalling
helpers, and that's all.

Reviewed-by: Markus Armbruster <armbru@redhat.com>


diff -rup qapi-gen-efcd0ec14b/commit qapi-gen-e46cbd9158/commit
--- qapi-gen-efcd0ec14b/commit	2023-04-06 07:50:53.212606061 +0200
+++ qapi-gen-e46cbd9158/commit	2023-04-06 07:49:49.254991726 +0200
@@ -1 +1 @@
-efcd0ec14b Merge tag 'misc-fixes-20230330' of https://github.com/philmd/qemu into staging
+e46cbd9158 qapi-gen: mark coroutine QMP command functions as coroutine_fn
diff -rup qapi-gen-efcd0ec14b/qapi-commands-block-core.c qapi-gen-e46cbd9158/qapi-commands-block-core.c
--- qapi-gen-efcd0ec14b/qapi-commands-block-core.c	2023-04-06 07:50:41.951850170 +0200
+++ qapi-gen-e46cbd9158/qapi-commands-block-core.c	2023-04-06 07:49:36.071277209 +0200
@@ -208,7 +208,7 @@ out:
     visit_free(v);
 }
 
-void qmp_marshal_block_resize(QDict *args, QObject **ret, Error **errp)
+void coroutine_fn qmp_marshal_block_resize(QDict *args, QObject **ret, Error **errp)
 {
     Error *err = NULL;
     bool ok = false;
diff -rup qapi-gen-efcd0ec14b/qapi-commands-block-core.h qapi-gen-e46cbd9158/qapi-commands-block-core.h
--- qapi-gen-efcd0ec14b/qapi-commands-block-core.h	2023-04-06 07:50:41.951850170 +0200
+++ qapi-gen-e46cbd9158/qapi-commands-block-core.h	2023-04-06 07:49:36.071277209 +0200
@@ -25,8 +25,8 @@ BlockStatsList *qmp_query_blockstats(boo
 void qmp_marshal_query_blockstats(QDict *args, QObject **ret, Error **errp);
 BlockJobInfoList *qmp_query_block_jobs(Error **errp);
 void qmp_marshal_query_block_jobs(QDict *args, QObject **ret, Error **errp);
-void qmp_block_resize(const char *device, const char *node_name, int64_t size, Error **errp);
-void qmp_marshal_block_resize(QDict *args, QObject **ret, Error **errp);
+void coroutine_fn qmp_block_resize(const char *device, const char *node_name, int64_t size, Error **errp);
+void coroutine_fn qmp_marshal_block_resize(QDict *args, QObject **ret, Error **errp);
 void qmp_blockdev_snapshot_sync(const char *device, const char *node_name, const char *snapshot_file, const char *snapshot_node_name, const char *format, bool has_mode, NewImageMode mode, Error **errp);
 void qmp_marshal_blockdev_snapshot_sync(QDict *args, QObject **ret, Error **errp);
 void qmp_blockdev_snapshot(const char *node, const char *overlay, Error **errp);
diff -rup qapi-gen-efcd0ec14b/qapi-commands-ui.c qapi-gen-e46cbd9158/qapi-commands-ui.c
--- qapi-gen-efcd0ec14b/qapi-commands-ui.c	2023-04-06 07:50:41.953850127 +0200
+++ qapi-gen-e46cbd9158/qapi-commands-ui.c	2023-04-06 07:49:36.073277166 +0200
@@ -107,7 +107,7 @@ out:
     visit_free(v);
 }
 
-void qmp_marshal_screendump(QDict *args, QObject **ret, Error **errp)
+void coroutine_fn qmp_marshal_screendump(QDict *args, QObject **ret, Error **errp)
 {
     Error *err = NULL;
     bool ok = false;
diff -rup qapi-gen-efcd0ec14b/qapi-commands-ui.h qapi-gen-e46cbd9158/qapi-commands-ui.h
--- qapi-gen-efcd0ec14b/qapi-commands-ui.h	2023-04-06 07:50:41.953850127 +0200
+++ qapi-gen-e46cbd9158/qapi-commands-ui.h	2023-04-06 07:49:36.073277166 +0200
@@ -21,8 +21,8 @@ void qmp_set_password(SetPasswordOptions
 void qmp_marshal_set_password(QDict *args, QObject **ret, Error **errp);
 void qmp_expire_password(ExpirePasswordOptions *arg, Error **errp);
 void qmp_marshal_expire_password(QDict *args, QObject **ret, Error **errp);
-void qmp_screendump(const char *filename, const char *device, bool has_head, int64_t head, bool has_format, ImageFormat format, Error **errp);
-void qmp_marshal_screendump(QDict *args, QObject **ret, Error **errp);
+void coroutine_fn qmp_screendump(const char *filename, const char *device, bool has_head, int64_t head, bool has_format, ImageFormat format, Error **errp);
+void coroutine_fn qmp_marshal_screendump(QDict *args, QObject **ret, Error **errp);
 #if defined(CONFIG_SPICE)
 SpiceInfo *qmp_query_spice(Error **errp);
 void qmp_marshal_query_spice(QDict *args, QObject **ret, Error **errp);
diff -rup qapi-gen-efcd0ec14b/test-qapi-commands.c qapi-gen-e46cbd9158/test-qapi-commands.c
--- qapi-gen-efcd0ec14b/test-qapi-commands.c	2023-04-06 07:50:42.146845943 +0200
+++ qapi-gen-e46cbd9158/test-qapi-commands.c	2023-04-06 07:49:36.286272553 +0200
@@ -213,7 +213,7 @@ out:
     visit_free(v);
 }
 
-void qmp_marshal_coroutine_cmd(QDict *args, QObject **ret, Error **errp)
+void coroutine_fn qmp_marshal_coroutine_cmd(QDict *args, QObject **ret, Error **errp)
 {
     Error *err = NULL;
     bool ok = false;
diff -rup qapi-gen-efcd0ec14b/test-qapi-commands.h qapi-gen-e46cbd9158/test-qapi-commands.h
--- qapi-gen-efcd0ec14b/test-qapi-commands.h	2023-04-06 07:50:42.146845943 +0200
+++ qapi-gen-e46cbd9158/test-qapi-commands.h	2023-04-06 07:49:36.286272553 +0200
@@ -26,8 +26,8 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne
 void qmp_marshal_user_def_cmd2(QDict *args, QObject **ret, Error **errp);
 void qmp_cmd_success_response(Error **errp);
 void qmp_marshal_cmd_success_response(QDict *args, QObject **ret, Error **errp);
-void qmp_coroutine_cmd(Error **errp);
-void qmp_marshal_coroutine_cmd(QDict *args, QObject **ret, Error **errp);
+void coroutine_fn qmp_coroutine_cmd(Error **errp);
+void coroutine_fn qmp_marshal_coroutine_cmd(QDict *args, QObject **ret, Error **errp);
 int64_t qmp_guest_get_time(int64_t a, bool has_b, int64_t b, Error **errp);
 void qmp_marshal_guest_get_time(QDict *args, QObject **ret, Error **errp);
 QObject *qmp_guest_sync(QObject *arg, Error **errp);