* [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn
@ 2022-10-13 9:35 Paolo Bonzini
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Bonzini @ 2022-10-13 9:35 UTC (permalink / raw)
To: qemu-devel; +Cc: afaria, qemu-block, armbru
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.
Unfortunately, doing so causes lots of files to fail to compile because
they do not include qemu/coroutine.h; which in principle is legitimate
because the files could be only dealing with non-coroutine commands.
There are three ways to deal with this:
- include qemu/coroutine.h in all the files that include the qapi-commands-*
headers. This would be a large change and in many case unnecessary,
because only very few files deal with coroutine commands
- include qemu/coroutine.h from the headers themselves. This is
ugly for the same reason, and also because headers-including-headers
make it harder to avoid world rebuilds
- only define the affected prototypes if coroutine_fn is defined,
meaning that the .c file has already included qemu/coroutine.h.
This is what the patch goes for.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/qapi/commands.py | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 38ca38a7b9..31833f172f 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:
+ 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:
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('''
@@ -314,6 +322,7 @@ def _begin_user_module(self, name: str) -> None:
#include "qapi/qmp/qdict.h"
#include "qapi/dealloc-visitor.h"
#include "qapi/error.h"
+#include "qemu/coroutine.h"
#include "%(visit)s.h"
#include "%(commands)s.h"
@@ -388,10 +397,15 @@ 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))
+ if coroutine:
+ self._genh.add('#ifdef coroutine_fn\n')
+ self._genh.add(gen_command_decl(name, arg_type, boxed,
+ ret_type, coroutine))
+ self._genh.add(gen_marshal_decl(name, coroutine))
+ if coroutine:
+ self._genh.add('#endif\n')
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'):
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn
@ 2023-04-05 15:02 Paolo Bonzini
2023-04-06 5:59 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2023-04-05 15:02 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
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>
---
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:
+ 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:
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'):
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn
2023-04-05 15:02 [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn Paolo Bonzini
@ 2023-04-06 5:59 ` Markus Armbruster
0 siblings, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2023-04-06 5:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, Kevin Wolf
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);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-04-06 6:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-05 15:02 [PATCH] qapi-gen: mark coroutine QMP command functions as coroutine_fn Paolo Bonzini
2023-04-06 5:59 ` Markus Armbruster
-- strict thread matches above, loose matches on Subject: below --
2022-10-13 9:35 Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).