Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
> We are going to generate trace events for qmp commands. We should
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> Now implement that possibility in gen_commands() function.
>
> The functionality will be used in the following commit, and now comment
> in _begin_user_module() about c_name() is a bit premature.
Neglects to explain why it needs to be optional.
Suggest
qapi/commands: Optionally generate trace for QMP commands
Add trace generation disabled. The next commit will enable it for
qapi/, but not for qga/ and tests/. Making it work for the latter two
would involve some Meson hackery to ensure we generate the
trace-events files before trace-tool uses them. Since we don't
actually support tracing there, we'll bypass that problem.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
> scripts/qapi/main.py | 2 +-
> 2 files changed, 88 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21001bbd6b..166bf5dcbc 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -53,7 +53,8 @@ def gen_command_decl(name: str,
> def gen_call(name: str,
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool,
> - ret_type: Optional[QAPISchemaType]) -> str:
> + ret_type: Optional[QAPISchemaType],
> + add_trace_events: bool) -> str:
> ret = ''
>
> argstr = ''
> @@ -71,21 +72,67 @@ def gen_call(name: str,
> if ret_type:
> lhs = 'retval = '
>
> - ret = mcgen('''
> + name = c_name(name)
> + upper = name.upper()
>
> - %(lhs)sqmp_%(c_name)s(%(args)s&err);
> - error_propagate(errp, err);
> -''',
> - c_name=c_name(name), args=argstr, lhs=lhs)
> - if ret_type:
> + if add_trace_events:
> ret += mcgen('''
> +
> + if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
> + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> +
> + trace_qmp_enter_%(name)s(req_json->str);
> + }
> + ''',
> + upper=upper, name=name)
> +
> + ret += mcgen('''
> +
> + %(lhs)sqmp_%(name)s(%(args)s&err);
> +''',
> + name=name, args=argstr, lhs=lhs)
> +
> + ret += mcgen('''
> if (err) {
> +''')
> +
> + if add_trace_events:
> + ret += mcgen('''
> + trace_qmp_exit_%(name)s(error_get_pretty(err), false);
> +''',
> + name=name)
> +
> + ret += mcgen('''
> + error_propagate(errp, err);
> goto out;
> }
> +''')
Before the patch, we generate
error_propagate(errp, err);
and if there's any code between here and out:
if (err) {
goto out;
}
After the patch, we always generate
if (err) {
error_propagate(errp, err);
goto out;
}
But with trace generation enabled (next patch), it changes to
if (err) {
trace_qmp_exit_FOO(error_get_pretty(err), false);
error_propagate(errp, err);
goto out;
}
trace_qmp_exit_FOO("{}", true);
Okay.
> +
> + if ret_type:
> + ret += mcgen('''
>
> qmp_marshal_output_%(c_name)s(retval, ret, errp);
> ''',
> c_name=ret_type.c_name())
> +
> + if add_trace_events:
> + if ret_type:
> + ret += mcgen('''
> +
> + if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
> + g_autoptr(GString) ret_json = qobject_to_json(*ret);
> +
> + trace_qmp_exit_%(name)s(ret_json->str, true);
> + }
> + ''',
> + upper=upper, name=name)
> + else:
> + ret += mcgen('''
> +
> + trace_qmp_exit_%(name)s("{}", true);
> + ''',
> + name=name)
> +
> return ret
>
>
> @@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
> proto=build_marshal_proto(name))
>
>
> +def gen_trace(name: str) -> str:
> + return mcgen('''
> +qmp_enter_%(name)s(const char *json) "%%s"
> +qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
> +''',
> + name=c_name(name))
> +
> +
> def gen_marshal(name: str,
> arg_type: Optional[QAPISchemaObjectType],
> boxed: bool,
> - ret_type: Optional[QAPISchemaType]) -> str:
> + ret_type: Optional[QAPISchemaType],
> + add_trace_events: bool) -> str:
> have_args = boxed or (arg_type and not arg_type.is_empty())
> if have_args:
> assert arg_type is not None
> @@ -180,7 +236,7 @@ def gen_marshal(name: str,
> }
> ''')
>
> - ret += gen_call(name, arg_type, boxed, ret_type)
> + ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>
> ret += mcgen('''
>
> @@ -238,11 +294,13 @@ def gen_register_command(name: str,
>
>
> class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> - def __init__(self, prefix: str):
> + def __init__(self, prefix: str, add_trace_events: bool):
> super().__init__(
> prefix, 'qapi-commands',
> - ' * Schema-defined QAPI/QMP commands', None, __doc__)
> + ' * Schema-defined QAPI/QMP commands', None, __doc__,
> + add_trace_events=add_trace_events)
> self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> + self.add_trace_events = add_trace_events
>
> def _begin_user_module(self, name: str) -> None:
> self._visited_ret_types[self._genc] = set()
> @@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>
> ''',
> commands=commands, visit=visit))
> +
> + if self.add_trace_events and commands != 'qapi-commands':
> + self._genc.add(mcgen('''
> +#include "trace/trace-qapi.h"
> +#include "qapi/qmp/qjson.h"
> +#include "trace/trace-%(nm)s_trace_events.h"
> +''',
> + nm=c_name(commands, protect=False)))
> + # We use c_name(commands, protect=False) to turn '-' into '_', to
> + # match .underscorify() in trace/meson.build
> +
> self._genh.add(mcgen('''
> #include "%(types)s.h"
>
> @@ -322,7 +391,10 @@ def visit_command(self,
> 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._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> + self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> + self.add_trace_events))
> + if self.add_trace_events:
> + self._gent.add(gen_trace(name))
> with self._temp_module('./init'):
> with ifcontext(ifcond, self._genh, self._genc):
> self._genc.add(gen_register_command(
> @@ -332,7 +404,8 @@ def visit_command(self,
>
> def gen_commands(schema: QAPISchema,
> output_dir: str,
> - prefix: str) -> None:
> - vis = QAPISchemaGenCommandVisitor(prefix)
> + prefix: str,
> + add_trace_events: bool) -> None:
> + vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
> schema.visit(vis)
> vis.write(output_dir)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..2e61409f04 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -49,7 +49,7 @@ def generate(schema_file: str,
> schema = QAPISchema(schema_file)
> gen_types(schema, output_dir, prefix, builtins)
> gen_visit(schema, output_dir, prefix, builtins)
> - gen_commands(schema, output_dir, prefix)
> + gen_commands(schema, output_dir, prefix, False)
> gen_events(schema, output_dir, prefix)
> gen_introspect(schema, output_dir, prefix, unmask)
Missing:
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index a3b5473089..feafed79b5 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1690,8 +1690,8 @@ Example::
}
retval = qmp_my_command(arg.arg1, &err);
- error_propagate(errp, err);
if (err) {
+ error_propagate(errp, err);
goto out;
}
