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

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

[Paolo Bonzini]

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 | 39 +++++++++++++++++++++++++++------------
 ui/console.c             |  1 +
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 38ca38a7b9..956a0d968f 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"
 
@@ -345,6 +354,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
                              c_prefix=c_name(self._prefix, protect=False)))
         self._genc.add(mcgen('''
 #include "qemu/osdep.h"
+#include "qemu/coroutine.h"
 #include "%(prefix)sqapi-commands.h"
 #include "%(prefix)sqapi-init-commands.h"
 
@@ -388,10 +398,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'):
diff --git a/ui/console.c b/ui/console.c
index 49da6a91df..61a3d62398 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -26,6 +26,7 @@
 #include "ui/console.h"
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
+#include "qemu/coroutine.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qemu/fifo8.h"
 #include "qemu/main-loop.h"
-- 
2.37.3

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Coroutine commands have to be declared as coroutine_fn, but the

Reminder, because I had to look this up...

A "coroutine command" is a command that has flag coroutine set, like so:

    { 'command': 'block_resize',
      'data': { '*device': 'str',
                '*node-name': 'str',
                'size': 'int' },
-->   'coroutine': true,
      'allow-preconfig': true }

This flag means "safe to run in coroutine context".

The QMP dispatcher runs in coroutine context (except for OOB commands,
but let's ignore those).  It executes coroutine commands directly, and
the others in a bottom half outside coroutine context.

See commit 04f22362f1 "qapi: Add a 'coroutine' flag for commands" and
commit 9ce44e2ce2 "qmp: Move dispatcher to a coroutine".

Only two coroutine commands exist so far.

> 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

Headers should be self-contained, i.e. include everything they need to
compile (except for qemu/osdep.h, which the .c include first).  The
first way would violate this.

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

Why can't we include qemu/coroutine.h only when the header actually
needs it?  I.e. when it declares a coroutine command.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>