| Message ID | 20220117201845.2438382-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace qmp commands | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > Add and option to generate trace events. We should generate both trace > events and trace-events files for further trace events code generation. Can you explain why we want trace generation to be optional? > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > scripts/qapi/commands.py | 91 ++++++++++++++++++++++++++++++++++------ > scripts/qapi/main.py | 10 +++-- > 2 files changed, 85 insertions(+), 16 deletions(-) > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > index 21001bbd6b..8cd1aa41ce 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,65 @@ 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)); Humor me: blank line between declarations and statements, please. > + 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) pycodestyle-3 gripes: scripts/qapi/commands.py:92:17: E128 continuation line under-indented for visual indent > + > + 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); Humor me: blank line between declarations and statements, please. > + trace_qmp_exit_%(name)s(ret_json->str, true); > + } > + ''', > + upper=upper, name=name) scripts/qapi/commands.py:126:22: E128 continuation line under-indented for visual indent > + else: > + ret += mcgen(''' > + > + trace_qmp_exit_%(name)s("{}", true); > + ''', > + name=name) > + > return ret The generated code changes like this when trace generation is enabled (next patch): @@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status( goto out; } + if (trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) { + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args)); + trace_qmp_enter_query_acpi_ospm_status(req_json->str); + } + retval = qmp_query_acpi_ospm_status(&err); if (err) { + trace_qmp_exit_query_acpi_ospm_status(error_get_pretty(err), false); error_propagate(errp, err); goto out; } qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp); + if (trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) { + g_autoptr(GString) ret_json = qobject_to_json(*ret); + trace_qmp_exit_query_acpi_ospm_status(ret_json->str, true); + } + out: visit_free(v); v = qapi_dealloc_visitor_new(); The trace_qmp_enter_query_acpi_ospm_status() and the second trace_qmp_exit_query_acpi_ospm_status() is guarded by trace_event_get_state_backends(), the first is not. Intentional? Have you considered something like @@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status( goto out; } + if (trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) { + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args)); + trace_qmp_enter_query_acpi_ospm_status(req_json->str); + } + retval = qmp_query_acpi_ospm_status(&err); if (err) { error_propagate(errp, err); goto out; } qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp); out: + if (trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) { + g_autoptr(GString) result_json + = qobject_to_json(err ? error_get_pretty(err) : *ret); + + trace_qmp_exit_query_acpi_ospm_status(ret_json->str, !err); + } + visit_free(v); v = qapi_dealloc_visitor_new(); > > > @@ -122,10 +167,17 @@ def gen_marshal_decl(name: str) -> str: > proto=build_marshal_proto(name)) > > > +def gen_trace(name: str) -> str: > + name = c_name(name) > + return f"""\ > +qmp_enter_{name}(const char *json) "%s"\n > +qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n""" Why not mcgen()? The generated FOO.trace-events look like this: $ cat bld-clang/qapi/qapi-commands-control.trace-events qmp_enter_qmp_capabilities(const char *json) "%s" qmp_exit_qmp_capabilities(const char *result, bool succeeded) "%s %d" qmp_enter_query_version(const char *json) "%s" qmp_exit_query_version(const char *result, bool succeeded) "%s %d" qmp_enter_query_commands(const char *json) "%s" qmp_exit_query_commands(const char *result, bool succeeded) "%s %d" qmp_enter_quit(const char *json) "%s" qmp_exit_quit(const char *result, bool succeeded) "%s %d" Either drop the blank lines, or put them between the pairs instead of within. I'd do the former. We generate lots of empty FOO.trace-events. I guess that's okay. > + scripts/qapi/commands.py:176:1: E302 expected 2 blank lines, found 1 > 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 +232,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 +290,12 @@ 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__) > 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 +314,15 @@ def _begin_user_module(self, name: str) -> None: > > ''', > commands=commands, visit=visit)) > + > + if self.add_trace_events and c_name(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))) Why c_name(commands), and not just commands? > + > self._genh.add(mcgen(''' > #include "%(types)s.h" > > @@ -322,7 +384,9 @@ 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)) > + 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 +396,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..7fab71401c 100644 > --- a/scripts/qapi/main.py > +++ b/scripts/qapi/main.py > @@ -32,7 +32,8 @@ def generate(schema_file: str, > output_dir: str, > prefix: str, > unmask: bool = False, > - builtins: bool = False) -> None: > + builtins: bool = False, > + add_trace_events: bool = False) -> None: > """ > Generate C code for the given schema into the target directory. > > @@ -49,7 +50,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, add_trace_events) > gen_events(schema, output_dir, prefix) > gen_introspect(schema, output_dir, prefix, unmask) > > @@ -74,6 +75,8 @@ def main() -> int: > parser.add_argument('-u', '--unmask-non-abi-names', action='store_true', > dest='unmask', > help="expose non-ABI names in introspection") > + parser.add_argument('--add-trace-events', action='store_true', > + help="add trace events to qmp marshals") > parser.add_argument('schema', action='store') > args = parser.parse_args() > > @@ -88,7 +91,8 @@ def main() -> int: > output_dir=args.output_dir, > prefix=args.prefix, > unmask=args.unmask, > - builtins=args.builtins) > + builtins=args.builtins, > + add_trace_events=args.add_trace_events) > except QAPIError as err: > print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr) > return 1 Missing: documentation for the tracing feature in docs/devel/qapi-code-gen.rst. We can talk about the level of detail last. Also missing is the example update: 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; }
18.01.2022 13:27, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Add and option to generate trace events. We should generate both trace >> events and trace-events files for further trace events code generation. > > Can you explain why we want trace generation to be optional? Because I failed make it work for tests and qga.. And seems there no good reason for it: there now trace events for now neither in tests nor in qga. I've now tried again. It doesn't work, as I understand, the problem is qga subdir goes after trace subdir, so when we generate trace headers, we didn't yet processed qga subdir. And I can't just move qga above trace: qga depends on qemutil variable so it should go after it. And if I put 'trace' subdir under qemuutil declaration it doesn't work too (seems because qemuutil depends on trace_ss).. So, supporting auto-generated trace points for qga qmp commands requires some deeper refactoring. > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> scripts/qapi/commands.py | 91 ++++++++++++++++++++++++++++++++++------ >> scripts/qapi/main.py | 10 +++-- >> 2 files changed, 85 insertions(+), 16 deletions(-) >> >> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py >> index 21001bbd6b..8cd1aa41ce 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,65 @@ 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)); > > Humor me: blank line between declarations and statements, please. > >> + 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) > > pycodestyle-3 gripes: > > scripts/qapi/commands.py:92:17: E128 continuation line under-indented for visual indent > >> + >> + 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); > > Humor me: blank line between declarations and statements, please. > >> + trace_qmp_exit_%(name)s(ret_json->str, true); >> + } >> + ''', >> + upper=upper, name=name) > > scripts/qapi/commands.py:126:22: E128 continuation line under-indented for visual indent > >> + else: >> + ret += mcgen(''' >> + >> + trace_qmp_exit_%(name)s("{}", true); >> + ''', >> + name=name) >> + >> return ret > > The generated code changes like this when trace generation is enabled > (next patch): > > @@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status( > goto out; > } > > + if (trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) { > + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args)); > + trace_qmp_enter_query_acpi_ospm_status(req_json->str); > + } > + > retval = qmp_query_acpi_ospm_status(&err); > if (err) { > + trace_qmp_exit_query_acpi_ospm_status(error_get_pretty(err), false); > error_propagate(errp, err); > goto out; > } > > qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp); > > + if (trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) { > + g_autoptr(GString) ret_json = qobject_to_json(*ret); > + trace_qmp_exit_query_acpi_ospm_status(ret_json->str, true); > + } > + > out: > visit_free(v); > v = qapi_dealloc_visitor_new(); > > The trace_qmp_enter_query_acpi_ospm_status() and the second > trace_qmp_exit_query_acpi_ospm_status() is guarded by > trace_event_get_state_backends(), the first is not. Intentional? Yes, I care to avoid json generation when trace event is disabled. > > Have you considered something like > > @@ -52,14 +55,25 @@ void qmp_marshal_query_acpi_ospm_status( > goto out; > } > > + if (trace_event_get_state_backends(TRACE_QMP_ENTER_QUERY_ACPI_OSPM_STATUS)) { > + g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args)); > + trace_qmp_enter_query_acpi_ospm_status(req_json->str); > + } > + > retval = qmp_query_acpi_ospm_status(&err); > if (err) { > error_propagate(errp, err); > goto out; > } > > qmp_marshal_output_ACPIOSTInfoList(retval, ret, errp); > > out: > + if (trace_event_get_state_backends(TRACE_QMP_EXIT_QUERY_ACPI_OSPM_STATUS)) { > + g_autoptr(GString) result_json > + = qobject_to_json(err ? error_get_pretty(err) : *ret); Hmm can qobject_to_json() work with simple string passed (returned by error_get_pretty() ? and it should not be automatically cleared.. And here err object is cleared (propagated to errp)... But we can move error_propagate() call after trace_qmp_exit_ , and it shoud work So, it should look like this: if (trace_event_get_state...) { g_autoptr(GString) result_json = NULL; const char *result_str; if (err) { result_str = error_get_pretty(err); } else { result_json = qobject_to_json(*ret); result_str = result_json->str; } trace_qmp_exit_query_acpi_ospm_status(result_str, !err); } error_propagate(errp, err); IMHO, my original variant looks nicer. > + > + trace_qmp_exit_query_acpi_ospm_status(ret_json->str, !err); > + } > + > visit_free(v); > v = qapi_dealloc_visitor_new(); > >> >> >> @@ -122,10 +167,17 @@ def gen_marshal_decl(name: str) -> str: >> proto=build_marshal_proto(name)) >> >> >> +def gen_trace(name: str) -> str: >> + name = c_name(name) >> + return f"""\ >> +qmp_enter_{name}(const char *json) "%s"\n >> +qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n""" > > Why not mcgen()? Hmm.. Here we don't need any indentation for sure. Do you think we still want mcgen for consistancy and not use f-string? > > The generated FOO.trace-events look like this: > > $ cat bld-clang/qapi/qapi-commands-control.trace-events > qmp_enter_qmp_capabilities(const char *json) "%s" > > qmp_exit_qmp_capabilities(const char *result, bool succeeded) "%s %d" > qmp_enter_query_version(const char *json) "%s" > > qmp_exit_query_version(const char *result, bool succeeded) "%s %d" > qmp_enter_query_commands(const char *json) "%s" > > qmp_exit_query_commands(const char *result, bool succeeded) "%s %d" > qmp_enter_quit(const char *json) "%s" > > qmp_exit_quit(const char *result, bool succeeded) "%s %d" > > Either drop the blank lines, or put them between the pairs instead of > within. I'd do the former. > > We generate lots of empty FOO.trace-events. I guess that's okay. > >> + > > scripts/qapi/commands.py:176:1: E302 expected 2 blank lines, found 1 > >> 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 +232,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 +290,12 @@ 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__) >> 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 +314,15 @@ def _begin_user_module(self, name: str) -> None: >> >> ''', >> commands=commands, visit=visit)) >> + >> + if self.add_trace_events and c_name(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))) > > Why c_name(commands), and not just commands? Because generated files has underscores instead of '-'. Looking at code, I think it's because underscorify() in trace/meson.build when we create group_name variable. > >> + >> self._genh.add(mcgen(''' >> #include "%(types)s.h" >> >> @@ -322,7 +384,9 @@ 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)) >> + 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 +396,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..7fab71401c 100644 >> --- a/scripts/qapi/main.py >> +++ b/scripts/qapi/main.py >> @@ -32,7 +32,8 @@ def generate(schema_file: str, >> output_dir: str, >> prefix: str, >> unmask: bool = False, >> - builtins: bool = False) -> None: >> + builtins: bool = False, >> + add_trace_events: bool = False) -> None: >> """ >> Generate C code for the given schema into the target directory. >> >> @@ -49,7 +50,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, add_trace_events) >> gen_events(schema, output_dir, prefix) >> gen_introspect(schema, output_dir, prefix, unmask) >> >> @@ -74,6 +75,8 @@ def main() -> int: >> parser.add_argument('-u', '--unmask-non-abi-names', action='store_true', >> dest='unmask', >> help="expose non-ABI names in introspection") >> + parser.add_argument('--add-trace-events', action='store_true', >> + help="add trace events to qmp marshals") >> parser.add_argument('schema', action='store') >> args = parser.parse_args() >> >> @@ -88,7 +91,8 @@ def main() -> int: >> output_dir=args.output_dir, >> prefix=args.prefix, >> unmask=args.unmask, >> - builtins=args.builtins) >> + builtins=args.builtins, >> + add_trace_events=args.add_trace_events) >> except QAPIError as err: >> print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr) >> return 1 > > Missing: documentation for the tracing feature in > docs/devel/qapi-code-gen.rst. We can talk about the level of detail > last. > > Also missing is the example update: > > 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; > } > >
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 18.01.2022 13:27, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> Add and option to generate trace events. We should generate both trace >>> events and trace-events files for further trace events code generation. >> >> Can you explain why we want trace generation to be optional? > > Because I failed make it work for tests and qga.. And seems there no good reason for it: there now trace events for now neither in tests nor in qga. > > I've now tried again. > > It doesn't work, as I understand, the problem is qga subdir goes after trace subdir, so when we generate trace headers, we didn't yet processed qga subdir. > > And I can't just move qga above trace: qga depends on qemutil variable so it should go after it. And if I put 'trace' subdir under qemuutil declaration it doesn't work too (seems because qemuutil depends on trace_ss).. > > So, supporting auto-generated trace points for qga qmp commands requires some deeper refactoring. Similar trouble with tests? The normal case seems to be "generate trace code", with an exception for cases where our build system defeats that. Agree? If yes, I'd prefer to default to "generate trace code", and have an option to suppress it, with suitable TODO comment(s) explaining why. [...] >>> @@ -122,10 +167,17 @@ def gen_marshal_decl(name: str) -> str: >>> proto=build_marshal_proto(name)) >>> >>> >>> +def gen_trace(name: str) -> str: >>> + name = c_name(name) >>> + return f"""\ >>> +qmp_enter_{name}(const char *json) "%s"\n >>> +qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n""" >> >> Why not mcgen()? > > Hmm.. Here we don't need any indentation for sure. Do you think we still want mcgen for consistancy and not use f-string? Let's stick to mcgen() for generating code. >> The generated FOO.trace-events look like this: >> >> $ cat bld-clang/qapi/qapi-commands-control.trace-events >> qmp_enter_qmp_capabilities(const char *json) "%s" >> >> qmp_exit_qmp_capabilities(const char *result, bool succeeded) "%s %d" >> qmp_enter_query_version(const char *json) "%s" >> >> qmp_exit_query_version(const char *result, bool succeeded) "%s %d" >> qmp_enter_query_commands(const char *json) "%s" >> >> qmp_exit_query_commands(const char *result, bool succeeded) "%s %d" >> qmp_enter_quit(const char *json) "%s" >> >> qmp_exit_quit(const char *result, bool succeeded) "%s %d" >> >> Either drop the blank lines, or put them between the pairs instead of >> within. I'd do the former. >> >> We generate lots of empty FOO.trace-events. I guess that's okay. >> >>> + >> >> scripts/qapi/commands.py:176:1: E302 expected 2 blank lines, found 1 >> >>> 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 +232,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 +290,12 @@ 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__) >>> 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 +314,15 @@ def _begin_user_module(self, name: str) -> None: >>> >>> ''', >>> commands=commands, visit=visit)) >>> + >>> + if self.add_trace_events and c_name(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))) >> >> Why c_name(commands), and not just commands? > > Because generated files has underscores instead of '-'. Looking at code, I think it's because underscorify() in trace/meson.build when we create group_name variable. I see. We first generate output modules like qapi/qapi-commands-machine-target.c qapi/qapi-commands-machine-target.h qapi/qapi-commands-machine-target.trace-events and then from the latter their trace code like trace/trace-qapi_commands_machine_target_trace_events.h trace/trace-qapi_commands_machine_target_trace_events.c These file names is ugly, but not this patch's fault. Normally, the foo/bar/*.c include "trace.h" (handwritten one-liner), which includes "trace/trace-foo_bar.h" generated from foo/bar/trace-events. Here, we include them directly, because we generate per file, not per directory. To actually match .underscorify(), you have to use c_name(commands, protect=False). Also add a comment that points to the .underscorify(). [...]
On 1/18/22 15:22, Markus Armbruster wrote: >> So, supporting auto-generated trace points for qga qmp commands requires some deeper refactoring. > Similar trouble with tests? > > The normal case seems to be "generate trace code", with an exception for > cases where our build system defeats that. Agree? More specifically, it's the lack of "include" statements: the only kind of include statement allowed by Meson is "include('foo/meson.build')" which is actually spelled "subdir('foo')". What this would require is akin to include('qga/meson.qapi.build') include('tests/qapi/meson.qapi.build') include('trace/meson.build') ... include('qga/meson.build') There has been an issue open in Meson forever about this: https://github.com/mesonbuild/meson/issues/375. Some discussion can be found in https://github.com/mesonbuild/meson/pull/5209. A somewhat ugly workaround would be something like subdir('qga/qapi') subdir('qapi') subdir('trace') ... subdir('qga') Or even, move the .json files for qemu-ga and tests to qapi/qga and qapi/tests respectively. That said, given that there's no tracing support in either trace nor in qemu-ga, I agree with Vladimir's assessment that there is no reason to do it. Paolo > If yes, I'd prefer to default to "generate trace code", and have an > option to suppress it, with suitable TODO comment(s) explaining why.
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 21001bbd6b..8cd1aa41ce 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,65 @@ 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 +167,17 @@ def gen_marshal_decl(name: str) -> str: proto=build_marshal_proto(name)) +def gen_trace(name: str) -> str: + name = c_name(name) + return f"""\ +qmp_enter_{name}(const char *json) "%s"\n +qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n""" + 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 +232,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 +290,12 @@ 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__) 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 +314,15 @@ def _begin_user_module(self, name: str) -> None: ''', commands=commands, visit=visit)) + + if self.add_trace_events and c_name(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))) + self._genh.add(mcgen(''' #include "%(types)s.h" @@ -322,7 +384,9 @@ 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)) + 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 +396,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..7fab71401c 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -32,7 +32,8 @@ def generate(schema_file: str, output_dir: str, prefix: str, unmask: bool = False, - builtins: bool = False) -> None: + builtins: bool = False, + add_trace_events: bool = False) -> None: """ Generate C code for the given schema into the target directory. @@ -49,7 +50,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, add_trace_events) gen_events(schema, output_dir, prefix) gen_introspect(schema, output_dir, prefix, unmask) @@ -74,6 +75,8 @@ def main() -> int: parser.add_argument('-u', '--unmask-non-abi-names', action='store_true', dest='unmask', help="expose non-ABI names in introspection") + parser.add_argument('--add-trace-events', action='store_true', + help="add trace events to qmp marshals") parser.add_argument('schema', action='store') args = parser.parse_args() @@ -88,7 +91,8 @@ def main() -> int: output_dir=args.output_dir, prefix=args.prefix, unmask=args.unmask, - builtins=args.builtins) + builtins=args.builtins, + add_trace_events=args.add_trace_events) except QAPIError as err: print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr) return 1
Add and option to generate trace events. We should generate both trace events and trace-events files for further trace events code generation. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- scripts/qapi/commands.py | 91 ++++++++++++++++++++++++++++++++++------ scripts/qapi/main.py | 10 +++-- 2 files changed, 85 insertions(+), 16 deletions(-)