| Message ID | 20220121162234.2707906-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | trace qmp commands | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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; }
25.01.2022 13:02, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@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. > Thanks, sounds good. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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; > } > > Ah right. I fix doc in the next commit, but you are right, the behavior for disabled trace events is modified already in this commit.
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; } +''') + + 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)
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. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------ scripts/qapi/main.py | 2 +- 2 files changed, 88 insertions(+), 15 deletions(-)