qapi: Add a 'coroutine' flag for commands
This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.
The documentation of the new flag pretends that this flag is already
used as intended, which it isn't yet after this patch. We'll implement
this in another patch in this series.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20201005155855.256490-9-kwolf@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 5fc67c9..4a41e36 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -472,6 +472,7 @@
'*gen': false,
'*allow-oob': true,
'*allow-preconfig': true,
+ '*coroutine': true,
'*if': COND,
'*features': FEATURES }
@@ -596,6 +597,34 @@
QMP is available before the machine is built only when QEMU was
started with --preconfig.
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine. It defaults to false. If it is true,
+the command handler is called from coroutine context and may yield while
+waiting for an external event (such as I/O completion) in order to avoid
+blocking the guest and other background operations.
+
+Coroutine safety can be hard to prove, similar to thread safety. Common
+pitfalls are:
+
+- The global mutex isn't held across qemu_coroutine_yield(), so
+ operations that used to assume that they execute atomically may have
+ to be more careful to protect against changes in the global state.
+
+- Nested event loops (AIO_WAIT_WHILE() etc.) are problematic in
+ coroutine context and can easily lead to deadlocks. They should be
+ replaced by yielding and reentering the coroutine when the condition
+ becomes false.
+
+Since the command handler may assume coroutine context, any callers
+other than the QMP dispatcher must also call it in coroutine context.
+In particular, HMP commands calling such a QMP command handler must
+enter coroutine context before calling the handler.
+
+It is an error to specify both 'coroutine': true and 'allow-oob': true
+for a command. We don't currently have a use case for both together and
+without a use case, it's not entirely clear what the semantics should
+be.
+
The optional 'if' member specifies a conditional. See "Configuring
the schema" below for more on this.
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 6944ffa..e03abcb 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -330,7 +330,7 @@
def visit_command(self, name, info, ifcond, features, arg_type,
ret_type, gen, success_response, boxed, allow_oob,
- allow_preconfig):
+ allow_preconfig, coroutine):
doc = self._cur_doc
self._add_doc('Command',
self._nodes_for_arguments(doc,
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 0c2f467..9fd2b72 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -25,6 +25,7 @@
QCO_NO_SUCCESS_RESP = (1U << 0),
QCO_ALLOW_OOB = (1U << 1),
QCO_ALLOW_PRECONFIG = (1U << 2),
+ QCO_COROUTINE = (1U << 3),
} QmpCommandOptions;
typedef struct QmpCommand
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3cf9e11..6e6fc94 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -176,7 +176,8 @@
return ret
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+ coroutine):
options = []
if not success_response:
@@ -185,6 +186,8 @@
options += ['QCO_ALLOW_OOB']
if allow_preconfig:
options += ['QCO_ALLOW_PRECONFIG']
+ if coroutine:
+ options += ['QCO_COROUTINE']
if not options:
options = ['QCO_NO_OPTIONS']
@@ -267,7 +270,7 @@
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig):
+ allow_oob, allow_preconfig, coroutine):
if not gen:
return
# FIXME: If T is a user-defined type, the user is responsible
@@ -285,7 +288,8 @@
self._genh.add(gen_marshal_decl(name))
self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
self._regy.add(gen_register_command(name, success_response,
- allow_oob, allow_preconfig))
+ allow_oob, allow_preconfig,
+ coroutine))
def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2942520..a15c1fb 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -88,10 +88,17 @@
if key in expr and expr[key] is not False:
raise QAPISemError(
info, "flag '%s' may only use false value" % key)
- for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+ for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
if key in expr and expr[key] is not True:
raise QAPISemError(
info, "flag '%s' may only use true value" % key)
+ if 'allow-oob' in expr and 'coroutine' in expr:
+ # This is not necessarily a fundamental incompatibility, but
+ # we don't have a use case and the desired semantics isn't
+ # obvious. The simplest solution is to forbid it until we get
+ # a use case for it.
+ raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
+ "are incompatible")
def check_if(expr, info, source):
@@ -342,7 +349,7 @@
['command'],
['data', 'returns', 'boxed', 'if', 'features',
'gen', 'success-response', 'allow-oob',
- 'allow-preconfig'])
+ 'allow-preconfig', 'coroutine'])
normalize_members(expr.get('data'))
check_command(expr, info)
elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 23652be..5907b09 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -216,7 +216,7 @@
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig):
+ allow_oob, allow_preconfig, coroutine):
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
obj = {'arg-type': self._use_type(arg_type),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 78309a00f..d1307ec 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -128,7 +128,7 @@
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig):
+ allow_oob, allow_preconfig, coroutine):
pass
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
@@ -713,7 +713,8 @@
def __init__(self, name, info, doc, ifcond, features,
arg_type, ret_type,
- gen, success_response, boxed, allow_oob, allow_preconfig):
+ gen, success_response, boxed, allow_oob, allow_preconfig,
+ coroutine):
super().__init__(name, info, doc, ifcond, features)
assert not arg_type or isinstance(arg_type, str)
assert not ret_type or isinstance(ret_type, str)
@@ -726,6 +727,7 @@
self.boxed = boxed
self.allow_oob = allow_oob
self.allow_preconfig = allow_preconfig
+ self.coroutine = coroutine
def check(self, schema):
super().check(schema)
@@ -768,7 +770,8 @@
visitor.visit_command(
self.name, self.info, self.ifcond, self.features,
self.arg_type, self.ret_type, self.gen, self.success_response,
- self.boxed, self.allow_oob, self.allow_preconfig)
+ self.boxed, self.allow_oob, self.allow_preconfig,
+ self.coroutine)
class QAPISchemaEvent(QAPISchemaEntity):
@@ -1074,6 +1077,7 @@
boxed = expr.get('boxed', False)
allow_oob = expr.get('allow-oob', False)
allow_preconfig = expr.get('allow-preconfig', False)
+ coroutine = expr.get('coroutine', False)
ifcond = expr.get('if')
features = self._make_features(expr.get('features'), info)
if isinstance(data, OrderedDict):
@@ -1086,7 +1090,8 @@
self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
data, rets,
gen, success_response,
- boxed, allow_oob, allow_preconfig))
+ boxed, allow_oob, allow_preconfig,
+ coroutine))
def _def_event(self, expr, info, doc):
name = expr['event']
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index f08c902..1f222a7 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -142,6 +142,7 @@
'nested-struct-data.json',
'nested-struct-data-invalid-dict.json',
'non-objects.json',
+ 'oob-coroutine.json',
'oob-test.json',
'allow-preconfig-test.json',
'pragma-doc-required-crap.json',
diff --git a/tests/qapi-schema/oob-coroutine.err b/tests/qapi-schema/oob-coroutine.err
new file mode 100644
index 0000000..c01a499
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.err
@@ -0,0 +1,2 @@
+oob-coroutine.json: In command 'oob-command-1':
+oob-coroutine.json:2: flags 'allow-oob' and 'coroutine' are incompatible
diff --git a/tests/qapi-schema/oob-coroutine.json b/tests/qapi-schema/oob-coroutine.json
new file mode 100644
index 0000000..0f67663
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.json
@@ -0,0 +1,2 @@
+# Check that incompatible flags allow-oob and coroutine are rejected
+{ 'command': 'oob-command-1', 'allow-oob': true, 'coroutine': true }
diff --git a/tests/qapi-schema/oob-coroutine.out b/tests/qapi-schema/oob-coroutine.out
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.out
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 3a9f2cb..63f92ad 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -148,6 +148,7 @@
'returns': 'UserDefTwo' }
{ 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
# Returning a non-dictionary requires a name from the whitelist
{ 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 891b410..8868ca0 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -203,6 +203,8 @@
gen=True success_response=True boxed=False oob=False preconfig=False
command cmd-success-response None -> None
gen=True success_response=False boxed=False oob=False preconfig=False
+command coroutine-cmd None -> None
+ gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True
object q_obj_guest-get-time-arg
member a: int optional=False
member b: int optional=True
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index f396b47..e8db9d0 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -68,12 +68,13 @@
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig):
+ allow_oob, allow_preconfig, coroutine):
print('command %s %s -> %s'
% (name, arg_type and arg_type.name,
ret_type and ret_type.name))
- print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
- % (gen, success_response, boxed, allow_oob, allow_preconfig))
+ print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
+ % (gen, success_response, boxed, allow_oob, allow_preconfig,
+ " coroutine=True" if coroutine else ""))
self._print_if(ifcond)
self._print_features(features)
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 5f1b245..d3413bf 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -36,6 +36,10 @@
{
}
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
Empty2 *qmp_user_def_cmd0(Error **errp)
{
return g_new0(Empty2, 1);
