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

[Markus Armbruster <armbru@redhat.com>]

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