Re: [PATCH v3 2/3] scripts/qapi-gen.py: add --add-trace-events option

[Vladimir Sementsov-Ogievskiy]

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;
>           }
>   


--
Best regards,
Vladimir