[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_
|
From: |
Markus Armbruster |
|
Subject: |
Re: [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn |
|
Date: |
Thu, 06 Apr 2023 07:59:57 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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);