[PATCH v3 0/3] trace qmp commands

[PATCH v3 0/3] trace qmp commands

[Vladimir Sementsov-Ogievskiy]

Hi all!

This series aims to add trace points for each qmp command with help of
qapi code generator.

v3:
 - don't drop old trace events
 - make pair of qmp_enter_ and qmp_exit_ trace events
 - improve patch splitting
 - use term "trace events" constantly instead of "trace points"
 - add comment on subdir ordering to last commit

Vladimir Sementsov-Ogievskiy (3):
  scripts/qapi/gen.py: add .trace-events file for module
  scripts/qapi-gen.py: add --add-trace-events option
  meson: generate trace events for qmp commands

 meson.build              |  5 +++
 qapi/meson.build         |  9 +++-
 scripts/qapi/commands.py | 91 ++++++++++++++++++++++++++++++++++------
 scripts/qapi/gen.py      | 13 ++++--
 scripts/qapi/main.py     | 10 +++--
 trace/meson.build        | 11 +++--
 6 files changed, 116 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] scripts/qapi/gen.py: add .trace-events file for module

[Vladimir Sementsov-Ogievskiy]

We are going to generate trace events for qmp commands. We should
generate both trace events and trace-events.

For now, add .trace-events file object, to be filled in further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/qapi/gen.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 995a97d2b8..605b3fe68a 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -251,7 +251,7 @@ def __init__(self,
         self._builtin_blurb = builtin_blurb
         self._pydoc = pydoc
         self._current_module: Optional[str] = None
-        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
+        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
         self._main_module: Optional[str] = None
 
     @property
@@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
         assert self._current_module is not None
         return self._module[self._current_module][1]
 
+    @property
+    def _gent(self) -> QAPIGen:
+        assert self._current_module is not None
+        return self._module[self._current_module][2]
+
     @staticmethod
     def _module_dirname(name: str) -> str:
         if QAPISchemaModule.is_user_module(name):
@@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
         basename = self._module_filename(self._what, name)
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
-        self._module[name] = (genc, genh)
+        gent = QAPIGen(basename + '.trace-events')
+        self._module[name] = (genc, genh, gent)
         self._current_module = name
 
     @contextmanager
@@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
         self._current_module = old_module
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
-        for name, (genc, genh) in self._module.items():
+        for name, (genc, genh, gent) in self._module.items():
             if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
                 continue
             genc.write(output_dir)
             genh.write(output_dir)
+            gent.write(output_dir)
 
     def _begin_builtin_module(self) -> None:
         pass
-- 
2.31.1

Re: [PATCH v3 1/3] scripts/qapi/gen.py: add .trace-events file for module

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> We are going to generate trace events for qmp commands. We should
> generate both trace events and trace-events.

What do you mean to say with the second sentence?  "trace events" == the
calls of generated trace_FOO(), and "trace-events" == the input file for
tracetool?

> For now, add .trace-events file object, to be filled in further commit.

What is a file object?  See below.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/gen.py | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..605b3fe68a 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -251,7 +251,7 @@ def __init__(self,
>          self._builtin_blurb = builtin_blurb
>          self._pydoc = pydoc
>          self._current_module: Optional[str] = None
> -        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
>          self._main_module: Optional[str] = None
>  
>      @property
> @@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
>          assert self._current_module is not None
>          return self._module[self._current_module][1]
>  
> +    @property
> +    def _gent(self) -> QAPIGen:
> +        assert self._current_module is not None
> +        return self._module[self._current_module][2]
> +
>      @staticmethod
>      def _module_dirname(name: str) -> str:
>          if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
>          basename = self._module_filename(self._what, name)
>          genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>          genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> -        self._module[name] = (genc, genh)
> +        gent = QAPIGen(basename + '.trace-events')

Aha.  You're adding an output module FOO.trace-events for schema module
FOO.

We commonly use a single trace-events per directory, but I believe your
choice of one per module is simpler for the QAPI generator, and just
fine for tracing.

Please add

   class QAPIGenTrace(QAPIGen):
   
       def _top(self, fname):
           return (QAPIGen._top(self, fname)
                   + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')


> +        self._module[name] = (genc, genh, gent)
>          self._current_module = name
>  
>      @contextmanager
> @@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
>          self._current_module = old_module
>  
>      def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> -        for name, (genc, genh) in self._module.items():
> +        for name, (genc, genh, gent) in self._module.items():
>              if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>                  continue
>              genc.write(output_dir)
>              genh.write(output_dir)
> +            gent.write(output_dir)
>  
>      def _begin_builtin_module(self) -> None:
>          pass

Re: [PATCH v3 1/3] scripts/qapi/gen.py: add .trace-events file for module

[Vladimir Sementsov-Ogievskiy]

18.01.2022 11:38, 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 events and trace-events.
> 
> What do you mean to say with the second sentence?  "trace events" == the
> calls of generated trace_FOO(), and "trace-events" == the input file for
> tracetool?

Right. Agree, sounds veird

> 
>> For now, add .trace-events file object, to be filled in further commit.
> 
> What is a file object?  See below.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/qapi/gen.py | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 995a97d2b8..605b3fe68a 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -251,7 +251,7 @@ def __init__(self,
>>           self._builtin_blurb = builtin_blurb
>>           self._pydoc = pydoc
>>           self._current_module: Optional[str] = None
>> -        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
>>           self._main_module: Optional[str] = None
>>   
>>       @property
>> @@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
>>           assert self._current_module is not None
>>           return self._module[self._current_module][1]
>>   
>> +    @property
>> +    def _gent(self) -> QAPIGen:
>> +        assert self._current_module is not None
>> +        return self._module[self._current_module][2]
>> +
>>       @staticmethod
>>       def _module_dirname(name: str) -> str:
>>           if QAPISchemaModule.is_user_module(name):
>> @@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
>>           basename = self._module_filename(self._what, name)
>>           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>>           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
>> -        self._module[name] = (genc, genh)
>> +        gent = QAPIGen(basename + '.trace-events')
> 
> Aha.  You're adding an output module FOO.trace-events for schema module
> FOO.

OK thanks, I'll reword commit message with these terms)

> 
> We commonly use a single trace-events per directory, but I believe your
> choice of one per module is simpler for the QAPI generator, and just
> fine for tracing.
> 
> Please add
> 
>     class QAPIGenTrace(QAPIGen):
>     
>         def _top(self, fname):
>             return (QAPIGen._top(self, fname)
>                     + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
> 

OK

> 
>> +        self._module[name] = (genc, genh, gent)
>>           self._current_module = name
>>   
>>       @contextmanager
>> @@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
>>           self._current_module = old_module
>>   
>>       def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>> -        for name, (genc, genh) in self._module.items():
>> +        for name, (genc, genh, gent) in self._module.items():
>>               if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>>                   continue
>>               genc.write(output_dir)
>>               genh.write(output_dir)
>> +            gent.write(output_dir)
>>   
>>       def _begin_builtin_module(self) -> None:
>>           pass
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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(-)

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
-- 
2.31.1

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

[Markus Armbruster]

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

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

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

[Markus Armbruster]

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().

[...]

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

[Paolo Bonzini]

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.

[PATCH v3 3/3] meson: generate trace events for qmp commands

[Vladimir Sementsov-Ogievskiy]

1. Use --add-trace-events when generate qmp commands
2. Add corresponding .trace-events files as outputs in qapi_files
   custom target
3. Define global qapi_trace_events list of .trace-events file targets,
   to fill in trace/qapi.build and to use in trace/meson.build
4. In trace/meson.build use the new array as an additional source of
   .trace_events files to be processed

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 meson.build       |  5 +++++
 qapi/meson.build  |  9 ++++++++-
 trace/meson.build | 11 ++++++++---
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 762d7cee85..effd66e4c2 100644
--- a/meson.build
+++ b/meson.build
@@ -41,6 +41,7 @@ qemu_icondir = get_option('datadir') / 'icons'
 
 config_host_data = configuration_data()
 genh = []
+qapi_trace_events = []
 
 target_dirs = config_host['TARGET_DIRS'].split()
 have_linux_user = false
@@ -2554,6 +2555,10 @@ if 'CONFIG_VHOST_USER' in config_host
   vhost_user = libvhost_user.get_variable('vhost_user_dep')
 endif
 
+# Please keep ordering between 'qapi' and 'trace' subdirs:
+# We should first handle 'qapi' subdir, so that all
+# generated trace events be generated prior handling 'trace'
+# subdir.
 subdir('qapi')
 subdir('qobject')
 subdir('stubs')
diff --git a/qapi/meson.build b/qapi/meson.build
index c0c49c15e4..b48125f8da 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -114,6 +114,7 @@ foreach module : qapi_all_modules
       'qapi-events-@0@.h'.format(module),
       'qapi-commands-@0@.c'.format(module),
       'qapi-commands-@0@.h'.format(module),
+      'qapi-commands-@0@.trace-events'.format(module),
     ]
   endif
   if module.endswith('-target')
@@ -126,7 +127,7 @@ endforeach
 qapi_files = custom_target('shared QAPI source files',
   output: qapi_util_outputs + qapi_specific_outputs + qapi_nonmodule_outputs,
   input: [ files('qapi-schema.json') ],
-  command: [ qapi_gen, '-o', 'qapi', '-b', '@INPUT0@' ],
+  command: [ qapi_gen, '-o', 'qapi', '-b', '@INPUT0@', '--add-trace-events' ],
   depend_files: [ qapi_inputs, qapi_gen_depends ])
 
 # Now go through all the outputs and add them to the right sourceset.
@@ -137,6 +138,9 @@ foreach output : qapi_util_outputs
   if output.endswith('.h')
     genh += qapi_files[i]
   endif
+  if output.endswith('.trace-events')
+    qapi_trace_events += qapi_files[i]
+  endif
   util_ss.add(qapi_files[i])
   i = i + 1
 endforeach
@@ -145,6 +149,9 @@ foreach output : qapi_specific_outputs + qapi_nonmodule_outputs
   if output.endswith('.h')
     genh += qapi_files[i]
   endif
+  if output.endswith('.trace-events')
+    qapi_trace_events += qapi_files[i]
+  endif
   specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: qapi_files[i])
   i = i + 1
 endforeach
diff --git a/trace/meson.build b/trace/meson.build
index 573dd699c6..c4794a1f2a 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -2,10 +2,15 @@
 specific_ss.add(files('control-target.c'))
 
 trace_events_files = []
-foreach dir : [ '.' ] + trace_events_subdirs
-  trace_events_file = meson.project_source_root() / dir / 'trace-events'
+foreach item : [ '.' ] + trace_events_subdirs + qapi_trace_events
+  if item in qapi_trace_events
+    trace_events_file = item
+    group_name = item.full_path().split('/')[-1].underscorify()
+  else
+    trace_events_file = meson.project_source_root() / item / 'trace-events'
+    group_name = item == '.' ? 'root' : item.underscorify()
+  endif
   trace_events_files += [ trace_events_file ]
-  group_name = dir == '.' ? 'root' : dir.underscorify()
   group = '--group=' + group_name
   fmt = '@0@-' + group_name + '.@1@'
 
-- 
2.31.1

Re: [PATCH v3 3/3] meson: generate trace events for qmp commands

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 1. Use --add-trace-events when generate qmp commands
> 2. Add corresponding .trace-events files as outputs in qapi_files
>    custom target
> 3. Define global qapi_trace_events list of .trace-events file targets,
>    to fill in trace/qapi.build and to use in trace/meson.build
> 4. In trace/meson.build use the new array as an additional source of
>    .trace_events files to be processed
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  meson.build       |  5 +++++
>  qapi/meson.build  |  9 ++++++++-
>  trace/meson.build | 11 ++++++++---
>  3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 762d7cee85..effd66e4c2 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -41,6 +41,7 @@ qemu_icondir = get_option('datadir') / 'icons'
>  
>  config_host_data = configuration_data()
>  genh = []
> +qapi_trace_events = []
>  
>  target_dirs = config_host['TARGET_DIRS'].split()
>  have_linux_user = false
> @@ -2554,6 +2555,10 @@ if 'CONFIG_VHOST_USER' in config_host
>    vhost_user = libvhost_user.get_variable('vhost_user_dep')
>  endif
>  
> +# Please keep ordering between 'qapi' and 'trace' subdirs:
> +# We should first handle 'qapi' subdir, so that all
> +# generated trace events be generated prior handling 'trace'
> +# subdir.

I naively expect explicit dependencies to be used for ordering, but I'm
a Meson noob.  I'd like an ACK from a non-noob on this one.

>  subdir('qapi')
>  subdir('qobject')
>  subdir('stubs')
> diff --git a/qapi/meson.build b/qapi/meson.build
> index c0c49c15e4..b48125f8da 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -114,6 +114,7 @@ foreach module : qapi_all_modules
>        'qapi-events-@0@.h'.format(module),
>        'qapi-commands-@0@.c'.format(module),
>        'qapi-commands-@0@.h'.format(module),
> +      'qapi-commands-@0@.trace-events'.format(module),
>      ]
>    endif
>    if module.endswith('-target')
> @@ -126,7 +127,7 @@ endforeach
>  qapi_files = custom_target('shared QAPI source files',
>    output: qapi_util_outputs + qapi_specific_outputs + qapi_nonmodule_outputs,
>    input: [ files('qapi-schema.json') ],
> -  command: [ qapi_gen, '-o', 'qapi', '-b', '@INPUT0@' ],
> +  command: [ qapi_gen, '-o', 'qapi', '-b', '@INPUT0@', '--add-trace-events' ],
>    depend_files: [ qapi_inputs, qapi_gen_depends ])
>  
>  # Now go through all the outputs and add them to the right sourceset.
> @@ -137,6 +138,9 @@ foreach output : qapi_util_outputs
>    if output.endswith('.h')
>      genh += qapi_files[i]
>    endif
> +  if output.endswith('.trace-events')
> +    qapi_trace_events += qapi_files[i]
> +  endif
>    util_ss.add(qapi_files[i])
>    i = i + 1
>  endforeach
> @@ -145,6 +149,9 @@ foreach output : qapi_specific_outputs + qapi_nonmodule_outputs
>    if output.endswith('.h')
>      genh += qapi_files[i]
>    endif
> +  if output.endswith('.trace-events')
> +    qapi_trace_events += qapi_files[i]
> +  endif
>    specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: qapi_files[i])
>    i = i + 1
>  endforeach
> diff --git a/trace/meson.build b/trace/meson.build
> index 573dd699c6..c4794a1f2a 100644
> --- a/trace/meson.build
> +++ b/trace/meson.build
> @@ -2,10 +2,15 @@
>  specific_ss.add(files('control-target.c'))
>  
>  trace_events_files = []
> -foreach dir : [ '.' ] + trace_events_subdirs
> -  trace_events_file = meson.project_source_root() / dir / 'trace-events'
> +foreach item : [ '.' ] + trace_events_subdirs + qapi_trace_events
> +  if item in qapi_trace_events
> +    trace_events_file = item
> +    group_name = item.full_path().split('/')[-1].underscorify()
> +  else
> +    trace_events_file = meson.project_source_root() / item / 'trace-events'
> +    group_name = item == '.' ? 'root' : item.underscorify()
> +  endif
>    trace_events_files += [ trace_events_file ]
> -  group_name = dir == '.' ? 'root' : dir.underscorify()
>    group = '--group=' + group_name
>    fmt = '@0@-' + group_name + '.@1@'

Re: [PATCH v3 3/3] meson: generate trace events for qmp commands

[Paolo Bonzini]

On 1/18/22 11:30, Markus Armbruster wrote:
>> +# Please keep ordering between 'qapi' and 'trace' subdirs:
>> +# We should first handle 'qapi' subdir, so that all
>> +# generated trace events be generated prior handling 'trace'
>> +# subdir.
> I naively expect explicit dependencies to be used for ordering, but I'm
> a Meson noob.  I'd like an ACK from a non-noob on this one.
> 

The Make-time dependencies are just fine, but still the Meson language 
is imperative (with generally immutable objects in order to avoid 
aliasing horrors) and variables in Meson are all of the ":=" kind; 
there's no equivalent for Make's "=".  So you have to do

	subdir('qapi')
	subdir('trace')

in this order so that the variables defined by qapi/ are found in trace/.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

but I would replace the comment with:

# NOTE: the trace/ subdirectory needs the qapi_trace_events variable
# that is filled in by qapi/.

Paolo

Re: [PATCH v3 3/3] meson: generate trace events for qmp commands

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 1/18/22 11:30, Markus Armbruster wrote:
>>> +# Please keep ordering between 'qapi' and 'trace' subdirs:
>>> +# We should first handle 'qapi' subdir, so that all
>>> +# generated trace events be generated prior handling 'trace'
>>> +# subdir.
>> I naively expect explicit dependencies to be used for ordering, but I'm
>> a Meson noob.  I'd like an ACK from a non-noob on this one.
>> 
>
> The Make-time dependencies are just fine, but still the Meson language
> is imperative (with generally immutable objects in order to avoid 
> aliasing horrors) and variables in Meson are all of the ":=" kind;
> there's no equivalent for Make's "=".  So you have to do
>
> 	subdir('qapi')
> 	subdir('trace')
>
> in this order so that the variables defined by qapi/ are found in trace/.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> but I would replace the comment with:
>
> # NOTE: the trace/ subdirectory needs the qapi_trace_events variable
> # that is filled in by qapi/.
>
> Paolo

Thanks!

Please also have a look at Vladimir's "supporting auto-generated trace
points for qga qmp commands requires some deeper refactoring" in reply
to PATCH 2.