[PATCH v4 0/3] trace qmp commands

[PATCH v4 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.

v4:

01: - better commit message
    - add QAPIGenTrace subclass of QAPIGen with _top()
    - make trace module optional: we need it only for commands, not for
      other QAPI targets (like events). Avoid creating a lot of extera
      .trace-events files.

02: - As we want to generate traces by default, in this patch we only
      update gen_commands() function.
    - fix style
    - add comment about c_name usage, add protect=False

03: - Now option disables trace generation, so option is added in this
      patch
    - Update documentation
    - Update comment on qapi / trace sequence


Vladimir Sementsov-Ogievskiy (3):
  scripts/qapi/gen.py: add FOO.trace-events output module
  scripts/qapi/commands: gen_commands(): add add_trace_events arg
  meson: generate trace events for qmp commands

 docs/devel/qapi-code-gen.rst |  23 +++++++-
 meson.build                  |   3 ++
 qapi/meson.build             |   7 +++
 qga/meson.build              |  11 +++-
 scripts/qapi/commands.py     | 101 ++++++++++++++++++++++++++++++-----
 scripts/qapi/gen.py          |  31 +++++++++--
 scripts/qapi/main.py         |  10 ++--
 tests/meson.build            |  11 +++-
 trace/meson.build            |  11 ++--
 9 files changed, 180 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[Vladimir Sementsov-Ogievskiy]

We are going to generate trace events for qmp commands. We should
generate both trace_*() function calls and trace-events files listing
events for trace generator.

So, add an output module FOO.trace-events for each FOO schema module.

Still, we'll need these .trace-events files only for
QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
So, make this possibility optional, to avoid generating extra empty
files for all other successors of QAPISchemaModularCVisitor.

We can't simply add the new feature directly to
QAPISchemaGenCommandVisitor: this means we'll have to reimplement
a kind of ._module / .write() functionality of parent class in the
successor, which seems worse than extending base class functionality.

Currently nobody set add_trace_events to True, so new functionality is
formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
in further commit.

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

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 995a97d2b8..def52f021e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -192,6 +192,11 @@ def _bottom(self) -> str:
         return guardend(self.fname)
 
 
+class QAPIGenTrace(QAPIGen):
+    def _top(self):
+        return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
+
+
 @contextmanager
 def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
     """
@@ -244,15 +249,18 @@ def __init__(self,
                  what: str,
                  user_blurb: str,
                  builtin_blurb: Optional[str],
-                 pydoc: str):
+                 pydoc: str,
+                 add_trace_events: bool = False):
         self._prefix = prefix
         self._what = what
         self._user_blurb = user_blurb
         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,
+                                      Optional[QAPIGenTrace]]] = {}
         self._main_module: Optional[str] = None
+        self.add_trace_events = add_trace_events
 
     @property
     def _genc(self) -> QAPIGenC:
@@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
         assert self._current_module is not None
         return self._module[self._current_module][1]
 
+    @property
+    def _gent(self) -> QAPIGenTrace:
+        assert self.add_trace_events
+        assert self._current_module is not None
+        gent = self._module[self._current_module][2]
+        assert gent is not None
+        return gent
+
     @staticmethod
     def _module_dirname(name: str) -> str:
         if QAPISchemaModule.is_user_module(name):
@@ -293,7 +309,12 @@ 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)
+        if self.add_trace_events:
+            gent = QAPIGenTrace(basename + '.trace-events')
+        else:
+            gent = None
+
+        self._module[name] = (genc, genh, gent)
         self._current_module = name
 
     @contextmanager
@@ -304,11 +325,13 @@ 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)
+            if gent is not None:
+                gent.write(output_dir)
 
     def _begin_builtin_module(self) -> None:
         pass
-- 
2.31.1

[PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg

[Vladimir Sementsov-Ogievskiy]

We are going to generate trace events for qmp commands. We should
generate both trace_*() function calls and trace-events files listing
events for trace generator.

Now implement that possibility in gen_commands() function.

The functionality will be used in the following commit, and now comment
in _begin_user_module() about c_name() is a bit premature.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
 scripts/qapi/main.py     |   2 +-
 2 files changed, 88 insertions(+), 15 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21001bbd6b..166bf5dcbc 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -53,7 +53,8 @@ def gen_command_decl(name: str,
 def gen_call(name: str,
              arg_type: Optional[QAPISchemaObjectType],
              boxed: bool,
-             ret_type: Optional[QAPISchemaType]) -> str:
+             ret_type: Optional[QAPISchemaType],
+             add_trace_events: bool) -> str:
     ret = ''
 
     argstr = ''
@@ -71,21 +72,67 @@ def gen_call(name: str,
     if ret_type:
         lhs = 'retval = '
 
-    ret = mcgen('''
+    name = c_name(name)
+    upper = name.upper()
 
-    %(lhs)sqmp_%(c_name)s(%(args)s&err);
-    error_propagate(errp, err);
-''',
-                c_name=c_name(name), args=argstr, lhs=lhs)
-    if ret_type:
+    if add_trace_events:
         ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
+        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+
+        trace_qmp_enter_%(name)s(req_json->str);
+    }
+    ''',
+                     upper=upper, name=name)
+
+    ret += mcgen('''
+
+    %(lhs)sqmp_%(name)s(%(args)s&err);
+''',
+                 name=name, args=argstr, lhs=lhs)
+
+    ret += mcgen('''
     if (err) {
+''')
+
+    if add_trace_events:
+        ret += mcgen('''
+        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
+''',
+                     name=name)
+
+    ret += mcgen('''
+        error_propagate(errp, err);
         goto out;
     }
+''')
+
+    if ret_type:
+        ret += mcgen('''
 
     qmp_marshal_output_%(c_name)s(retval, ret, errp);
 ''',
                      c_name=ret_type.c_name())
+
+    if add_trace_events:
+        if ret_type:
+            ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
+        g_autoptr(GString) ret_json = qobject_to_json(*ret);
+
+        trace_qmp_exit_%(name)s(ret_json->str, true);
+    }
+    ''',
+                         upper=upper, name=name)
+        else:
+            ret += mcgen('''
+
+    trace_qmp_exit_%(name)s("{}", true);
+    ''',
+                         name=name)
+
     return ret
 
 
@@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
                  proto=build_marshal_proto(name))
 
 
+def gen_trace(name: str) -> str:
+    return mcgen('''
+qmp_enter_%(name)s(const char *json) "%%s"
+qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
+''',
+                 name=c_name(name))
+
+
 def gen_marshal(name: str,
                 arg_type: Optional[QAPISchemaObjectType],
                 boxed: bool,
-                ret_type: Optional[QAPISchemaType]) -> str:
+                ret_type: Optional[QAPISchemaType],
+                add_trace_events: bool) -> str:
     have_args = boxed or (arg_type and not arg_type.is_empty())
     if have_args:
         assert arg_type is not None
@@ -180,7 +236,7 @@ def gen_marshal(name: str,
     }
 ''')
 
-    ret += gen_call(name, arg_type, boxed, ret_type)
+    ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
 
     ret += mcgen('''
 
@@ -238,11 +294,13 @@ def gen_register_command(name: str,
 
 
 class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
-    def __init__(self, prefix: str):
+    def __init__(self, prefix: str, add_trace_events: bool):
         super().__init__(
             prefix, 'qapi-commands',
-            ' * Schema-defined QAPI/QMP commands', None, __doc__)
+            ' * Schema-defined QAPI/QMP commands', None, __doc__,
+            add_trace_events=add_trace_events)
         self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
+        self.add_trace_events = add_trace_events
 
     def _begin_user_module(self, name: str) -> None:
         self._visited_ret_types[self._genc] = set()
@@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
 
 ''',
                              commands=commands, visit=visit))
+
+        if self.add_trace_events and commands != 'qapi-commands':
+            self._genc.add(mcgen('''
+#include "trace/trace-qapi.h"
+#include "qapi/qmp/qjson.h"
+#include "trace/trace-%(nm)s_trace_events.h"
+''',
+                                 nm=c_name(commands, protect=False)))
+            # We use c_name(commands, protect=False) to turn '-' into '_', to
+            # match .underscorify() in trace/meson.build
+
         self._genh.add(mcgen('''
 #include "%(types)s.h"
 
@@ -322,7 +391,10 @@ def visit_command(self,
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
             self._genh.add(gen_marshal_decl(name))
-            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
+                                       self.add_trace_events))
+            if self.add_trace_events:
+                self._gent.add(gen_trace(name))
         with self._temp_module('./init'):
             with ifcontext(ifcond, self._genh, self._genc):
                 self._genc.add(gen_register_command(
@@ -332,7 +404,8 @@ def visit_command(self,
 
 def gen_commands(schema: QAPISchema,
                  output_dir: str,
-                 prefix: str) -> None:
-    vis = QAPISchemaGenCommandVisitor(prefix)
+                 prefix: str,
+                 add_trace_events: bool) -> None:
+    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
     schema.visit(vis)
     vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..2e61409f04 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -49,7 +49,7 @@ def generate(schema_file: str,
     schema = QAPISchema(schema_file)
     gen_types(schema, output_dir, prefix, builtins)
     gen_visit(schema, output_dir, prefix, builtins)
-    gen_commands(schema, output_dir, prefix)
+    gen_commands(schema, output_dir, prefix, False)
     gen_events(schema, output_dir, prefix)
     gen_introspect(schema, output_dir, prefix, unmask)
 
-- 
2.31.1

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

[Vladimir Sementsov-Ogievskiy]

1. Add --no-trace-events to suppress trace events generation in some
   cases, and make trace events be generated by default.
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>
---
 docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
 meson.build                  |  3 +++
 qapi/meson.build             |  7 +++++++
 qga/meson.build              | 11 ++++++++++-
 scripts/qapi/main.py         | 10 +++++++---
 tests/meson.build            | 11 ++++++++++-
 trace/meson.build            | 11 ++++++++---
 7 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index a3b5473089..a3430740bd 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1619,7 +1619,10 @@ Code generated for commands
 
 These are the marshaling/dispatch functions for the commands defined
 in the schema.  The generated code provides qmp_marshal_COMMAND(), and
-declares qmp_COMMAND() that the user must implement.
+declares qmp_COMMAND() that the user must implement.  The generated code
+contains trace events code.  Corresponding .trace-events file with list
+of trace events is generated too, and should be parsed by trace generator
+later to generate trace event code, see `tracing <tracing.html>`.
 
 The following files are generated:
 
@@ -1630,6 +1633,9 @@ The following files are generated:
  ``$(prefix)qapi-commands.h``
      Function prototypes for the QMP commands specified in the schema
 
+ ``$(prefix)qapi-commands.trace-events``
+     Trace events file for trace generator, see `tracing <tracing.html>`.
+
  ``$(prefix)qapi-init-commands.h``
      Command initialization prototype
 
@@ -1689,14 +1695,27 @@ Example::
             goto out;
         }
 
+        if (trace_event_get_state_backends(TRACE_QMP_ENTER_MY_COMMAND)) {
+            g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+
+            trace_qmp_enter_my_command(req_json->str);
+        }
+
         retval = qmp_my_command(arg.arg1, &err);
-        error_propagate(errp, err);
         if (err) {
+            trace_qmp_exit_my_command(error_get_pretty(err), false);
+            error_propagate(errp, err);
             goto out;
         }
 
         qmp_marshal_output_UserDefOne(retval, ret, errp);
 
+        if (trace_event_get_state_backends(TRACE_QMP_EXIT_MY_COMMAND)) {
+            g_autoptr(GString) ret_json = qobject_to_json(*ret);
+
+            trace_qmp_exit_my_command(ret_json->str, true);
+        }
+
     out:
         visit_free(v);
         v = qapi_dealloc_visitor_new();
diff --git a/meson.build b/meson.build
index 833fd6bc4c..e0cfafe8d9 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
@@ -2557,6 +2558,8 @@ if 'CONFIG_VHOST_USER' in config_host
   vhost_user = libvhost_user.get_variable('vhost_user_dep')
 endif
 
+# NOTE: the trace/ subdirectory needs the qapi_trace_events variable
+# that is filled in by qapi/.
 subdir('qapi')
 subdir('qobject')
 subdir('stubs')
diff --git a/qapi/meson.build b/qapi/meson.build
index c0c49c15e4..656ef0e039 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')
@@ -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/qga/meson.build b/qga/meson.build
index cfb1fbc085..1f2818a1b9 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -15,10 +15,19 @@ qga_qapi_outputs = [
   'qga-qapi-visit.h',
 ]
 
+# We don't generate trace-events, just because it's not simple. For do it,
+# we also should add .trace-events file into qga_qapi_outputs, and than
+# add corresponding element of qga_qapi_files into qapi_trace_events
+# global list, which is processed than in trace/meson.build.
+# This means, that we'll have to move subdir('qga') above subdir('trace')
+# in root meson.build. But that in turn will break the dependency of
+# qga on qemuutil (which depends on trace_ss).
+# So, resolving these dependencies and drop --no-trace-events is a TODO.
 qga_qapi_files = custom_target('QGA QAPI files',
                                output: qga_qapi_outputs,
                                input: 'qapi-schema.json',
-                               command: [ qapi_gen, '-o', 'qga', '-p', 'qga-', '@INPUT0@' ],
+                               command: [ qapi_gen, '-o', 'qga', '-p', 'qga-', '@INPUT0@',
+                                          '--no-trace-events' ],
                                depend_files: qapi_gen_depends)
 
 qga_ss = ss.source_set()
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 2e61409f04..d53e5b800c 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, False)
+    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('--no-trace-events', action='store_true',
+                        help="suppress adding 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=not args.no_trace_events)
     except QAPIError as err:
         print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
         return 1
diff --git a/tests/meson.build b/tests/meson.build
index 3f3882748a..b4b95cc198 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -31,13 +31,22 @@ test_qapi_outputs = [
   'test-qapi-visit.h',
 ]
 
+# We don't generate trace-events, just because it's not simple. For do it,
+# we also should add .trace-events file into test_qapi_outputs, and than
+# add corresponding element of test_qapi_files into qapi_trace_events
+# global list, which is processed than in trace/meson.build.
+# This means, that we'll have to move subdir('tests') above subdir('trace')
+# in root meson.build. But that in turn will break the dependency of
+# tests on qemuutil (which depends on trace_ss).
+# So, resolving these dependencies and drop --no-trace-events is a TODO.
 test_qapi_files = custom_target('Test QAPI files',
                                 output: test_qapi_outputs,
                                 input: files('qapi-schema/qapi-schema-test.json',
                                              'qapi-schema/include/sub-module.json',
                                              'qapi-schema/sub-sub-module.json'),
                                 command: [ qapi_gen, '-o', meson.current_build_dir(),
-                                           '-b', '-p', 'test-', '@INPUT0@' ],
+                                           '-b', '-p', 'test-', '@INPUT0@',
+                                           '--no-trace-events' ],
                                 depend_files: qapi_gen_depends)
 
 # meson doesn't like generated output in other directories
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 v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[John Snow]

On Fri, Jan 21, 2022 at 11:22 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> We are going to generate trace events for qmp commands. We should
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> So, add an output module FOO.trace-events for each FOO schema module.
>
> Still, we'll need these .trace-events files only for
> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
> So, make this possibility optional, to avoid generating extra empty
> files for all other successors of QAPISchemaModularCVisitor.
>
> We can't simply add the new feature directly to
> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
> a kind of ._module / .write() functionality of parent class in the
> successor, which seems worse than extending base class functionality.
>
> Currently nobody set add_trace_events to True, so new functionality is
> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
> in further commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..def52f021e 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
>          return guardend(self.fname)
>
>
> +class QAPIGenTrace(QAPIGen):
> +    def _top(self):

-> str:

> +        return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
> +
> +
>  @contextmanager
>  def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>      """
> @@ -244,15 +249,18 @@ def __init__(self,
>                   what: str,
>                   user_blurb: str,
>                   builtin_blurb: Optional[str],
> -                 pydoc: str):
> +                 pydoc: str,
> +                 add_trace_events: bool = False):
>          self._prefix = prefix
>          self._what = what
>          self._user_blurb = user_blurb
>          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,
> +                                      Optional[QAPIGenTrace]]] = {}
>          self._main_module: Optional[str] = None
> +        self.add_trace_events = add_trace_events
>
>      @property
>      def _genc(self) -> QAPIGenC:
> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
>          assert self._current_module is not None
>          return self._module[self._current_module][1]
>
> +    @property
> +    def _gent(self) -> QAPIGenTrace:
> +        assert self.add_trace_events
> +        assert self._current_module is not None
> +        gent = self._module[self._current_module][2]
> +        assert gent is not None
> +        return gent
> +
>      @staticmethod
>      def _module_dirname(name: str) -> str:
>          if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +309,12 @@ 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: Optional[QAPIGenTrace]

> +        if self.add_trace_events:
> +            gent = QAPIGenTrace(basename + '.trace-events')
> +        else:
> +            gent = None
> +
> +        self._module[name] = (genc, genh, gent)
>          self._current_module = name
>
>      @contextmanager
> @@ -304,11 +325,13 @@ 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)
> +            if gent is not None:
> +                gent.write(output_dir)
>
>      def _begin_builtin_module(self) -> None:
>          pass
> --
> 2.31.1
>

... Sorry, I didn't finish typing this module 100%, so the scripts
aren't in the tree yet. I'll have to resume that project soon, I am
just trying to juggle a lot of things simultaneously. forgive me!

but, these should work:

> cd ~/src/qemu/scripts
> mypy --config-file=qapi/mypy.ini qapi/
> flake8 --config=qapi/.flake8 qapi/

pylint and isort has had some small regressions, so don't worry about
those as much:

> pylint --rcfile=qapi/pylintrc qapi/
************* Module qapi.events
qapi/events.py:112:8: C0103: Variable name "f" doesn't conform to
snake_case naming style (invalid-name)

--js

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

[John Snow]

On Fri, Jan 21, 2022 at 11:23 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 1. Add --no-trace-events to suppress trace events generation in some
>    cases, and make trace events be generated by default.
> 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>

patch 2 and 3 come out clean under the kinda-sorta-linting I've been doing:

Acked-by: John Snow <jsnow@redhat.com>

(No time to do a bigger review, I'm drowning in backlog ... you and
Markus know where to find me if you need something urgently, though.)

Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[Markus Armbruster]

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

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

QMP

> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> So, add an output module FOO.trace-events for each FOO schema module.
>
> Still, we'll need these .trace-events files only for
> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
> So, make this possibility optional, to avoid generating extra empty
> files for all other successors of QAPISchemaModularCVisitor.

Suggest to make this slightly less technical for easier reading:

  Since we're going to add trace events only to command marshallers,
  make the trace-events output optional, so we don't generate so many
  useless empty files.

> We can't simply add the new feature directly to
> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
> a kind of ._module / .write() functionality of parent class in the
> successor, which seems worse than extending base class functionality.

Do you mean something like

  The alternative would be adding the the new feature directly to
  QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
  ._module / .write() functionality of its parent class
  QAPISchemaModularCVisitor, which seems worse than extending the parent
  class.

?

If yes: I'm not sure about "worse".  But keeping it in the parent class
feels right to me anyway, as trace events could be useful in other child
classes, too.

> Currently nobody set add_trace_events to True, so new functionality is
> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor

Drop "formally".

> in further commit.

"in a further commit", or "in the next commit".

>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..def52f021e 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
>          return guardend(self.fname)
>  
>  
> +class QAPIGenTrace(QAPIGen):
> +    def _top(self):
> +        return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
> +
> +
>  @contextmanager
>  def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>      """
> @@ -244,15 +249,18 @@ def __init__(self,
>                   what: str,
>                   user_blurb: str,
>                   builtin_blurb: Optional[str],
> -                 pydoc: str):
> +                 pydoc: str,
> +                 add_trace_events: bool = False):

I'd prefer naming this gen_trace_events.  Happy to tweak this in my tree.

>          self._prefix = prefix
>          self._what = what
>          self._user_blurb = user_blurb
>          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,
> +                                      Optional[QAPIGenTrace]]] = {}
>          self._main_module: Optional[str] = None
> +        self.add_trace_events = add_trace_events

By convention, names of private attributes start with a single _.

>  
>      @property
>      def _genc(self) -> QAPIGenC:
> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
>          assert self._current_module is not None
>          return self._module[self._current_module][1]
>  
> +    @property
> +    def _gent(self) -> QAPIGenTrace:
> +        assert self.add_trace_events
> +        assert self._current_module is not None
> +        gent = self._module[self._current_module][2]
> +        assert gent is not None
> +        return gent
> +
>      @staticmethod
>      def _module_dirname(name: str) -> str:
>          if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +309,12 @@ 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)
> +        if self.add_trace_events:
> +            gent = QAPIGenTrace(basename + '.trace-events')
> +        else:
> +            gent = None
> +
> +        self._module[name] = (genc, genh, gent)
>          self._current_module = name
>  
>      @contextmanager
> @@ -304,11 +325,13 @@ 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)
> +            if gent is not None:
> +                gent.write(output_dir)
>  
>      def _begin_builtin_module(self) -> None:
>          pass

I wonder whether we really need a new __init__() parameter.  Could we
have ._gent() create the module on demand?  This is *not* a demand.

Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[Vladimir Sementsov-Ogievskiy]

25.01.2022 12:07, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> We are going to generate trace events for qmp commands. We should
> 
> QMP
> 
>> generate both trace_*() function calls and trace-events files listing
>> events for trace generator.
>>
>> So, add an output module FOO.trace-events for each FOO schema module.
>>
>> Still, we'll need these .trace-events files only for
>> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
>> So, make this possibility optional, to avoid generating extra empty
>> files for all other successors of QAPISchemaModularCVisitor.
> 
> Suggest to make this slightly less technical for easier reading:
> 
>    Since we're going to add trace events only to command marshallers,
>    make the trace-events output optional, so we don't generate so many
>    useless empty files.

Sounds good

> 
>> We can't simply add the new feature directly to
>> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
>> a kind of ._module / .write() functionality of parent class in the
>> successor, which seems worse than extending base class functionality.
> 
> Do you mean something like
> 
>    The alternative would be adding the the new feature directly to
>    QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
>    ._module / .write() functionality of its parent class
>    QAPISchemaModularCVisitor, which seems worse than extending the parent
>    class.
> 
> ?

Yes.

> 
> If yes: I'm not sure about "worse". 

Hmm. *shrug* ) I'm new to this code, that's why it seems unobvious to me, and that's why I'm afraid of deeper refactoring)

> But keeping it in the parent class
> feels right to me anyway, as trace events could be useful in other child
> classes, too.

If it is OK, we can simply drop this paragraph.

> 
>> Currently nobody set add_trace_events to True, so new functionality is
>> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
> 
> Drop "formally".
> 
>> in further commit.
> 
> "in a further commit", or "in the next commit".
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 995a97d2b8..def52f021e 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
>>           return guardend(self.fname)
>>   
>>   
>> +class QAPIGenTrace(QAPIGen):
>> +    def _top(self):
>> +        return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n'
>> +
>> +
>>   @contextmanager
>>   def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>>       """
>> @@ -244,15 +249,18 @@ def __init__(self,
>>                    what: str,
>>                    user_blurb: str,
>>                    builtin_blurb: Optional[str],
>> -                 pydoc: str):
>> +                 pydoc: str,
>> +                 add_trace_events: bool = False):
> 
> I'd prefer naming this gen_trace_events.  Happy to tweak this in my tree.
> 
>>           self._prefix = prefix
>>           self._what = what
>>           self._user_blurb = user_blurb
>>           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,
>> +                                      Optional[QAPIGenTrace]]] = {}
>>           self._main_module: Optional[str] = None
>> +        self.add_trace_events = add_trace_events
> 
> By convention, names of private attributes start with a single _.
> 
>>   
>>       @property
>>       def _genc(self) -> QAPIGenC:
>> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
>>           assert self._current_module is not None
>>           return self._module[self._current_module][1]
>>   
>> +    @property
>> +    def _gent(self) -> QAPIGenTrace:
>> +        assert self.add_trace_events
>> +        assert self._current_module is not None
>> +        gent = self._module[self._current_module][2]
>> +        assert gent is not None
>> +        return gent
>> +
>>       @staticmethod
>>       def _module_dirname(name: str) -> str:
>>           if QAPISchemaModule.is_user_module(name):
>> @@ -293,7 +309,12 @@ 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)
>> +        if self.add_trace_events:
>> +            gent = QAPIGenTrace(basename + '.trace-events')
>> +        else:
>> +            gent = None
>> +
>> +        self._module[name] = (genc, genh, gent)
>>           self._current_module = name
>>   
>>       @contextmanager
>> @@ -304,11 +325,13 @@ 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)
>> +            if gent is not None:
>> +                gent.write(output_dir)
>>   
>>       def _begin_builtin_module(self) -> None:
>>           pass
> 
> I wonder whether we really need a new __init__() parameter.  Could we
> have ._gent() create the module on demand?  This is *not* a demand.
> 

My first attempt to drop extra empty generated .trace-events files was to teach QAPIGenTrace() not to generate file when it is empty. But in this case some empty .trace-events for commands are not generated, and "include" line fails to compile. And at time when include line is generated, I don't know will corresponding .trace-events be empty or not. So I decided to make a new parameter for __init__()

-- 
Best regards,
Vladimir

Re: [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg

[Markus Armbruster]

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

> We are going to generate trace events for qmp commands. We should
> generate both trace_*() function calls and trace-events files listing
> events for trace generator.
>
> Now implement that possibility in gen_commands() function.
>
> The functionality will be used in the following commit, and now comment
> in _begin_user_module() about c_name() is a bit premature.

Neglects to explain why it needs to be optional.

Suggest

  qapi/commands: Optionally generate trace for QMP commands

  Add trace generation disabled.  The next commit will enable it for
  qapi/, but not for qga/ and tests/.  Making it work for the latter two
  would involve some Meson hackery to ensure we generate the
  trace-events files before trace-tool uses them.  Since we don't
  actually support tracing there, we'll bypass that problem.

>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
>  scripts/qapi/main.py     |   2 +-
>  2 files changed, 88 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21001bbd6b..166bf5dcbc 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -53,7 +53,8 @@ def gen_command_decl(name: str,
>  def gen_call(name: str,
>               arg_type: Optional[QAPISchemaObjectType],
>               boxed: bool,
> -             ret_type: Optional[QAPISchemaType]) -> str:
> +             ret_type: Optional[QAPISchemaType],
> +             add_trace_events: bool) -> str:
>      ret = ''
>  
>      argstr = ''
> @@ -71,21 +72,67 @@ def gen_call(name: str,
>      if ret_type:
>          lhs = 'retval = '
>  
> -    ret = mcgen('''
> +    name = c_name(name)
> +    upper = name.upper()
>  
> -    %(lhs)sqmp_%(c_name)s(%(args)s&err);
> -    error_propagate(errp, err);
> -''',
> -                c_name=c_name(name), args=argstr, lhs=lhs)
> -    if ret_type:
> +    if add_trace_events:
>          ret += mcgen('''
> +
> +    if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
> +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> +
> +        trace_qmp_enter_%(name)s(req_json->str);
> +    }
> +    ''',
> +                     upper=upper, name=name)
> +
> +    ret += mcgen('''
> +
> +    %(lhs)sqmp_%(name)s(%(args)s&err);
> +''',
> +                 name=name, args=argstr, lhs=lhs)
> +
> +    ret += mcgen('''
>      if (err) {
> +''')
> +
> +    if add_trace_events:
> +        ret += mcgen('''
> +        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
> +        error_propagate(errp, err);
>          goto out;
>      }
> +''')

Before the patch, we generate

    error_propagate(errp, err);

and if there's any code between here and out:

    if (err) {
        goto out;
    }

After the patch, we always generate

    if (err) {
        error_propagate(errp, err);
        goto out;
    }

But with trace generation enabled (next patch), it changes to

    if (err) {
        trace_qmp_exit_FOO(error_get_pretty(err), false);
        error_propagate(errp, err);
        goto out;
    }

    trace_qmp_exit_FOO("{}", true);

Okay.

> +
> +    if ret_type:
> +        ret += mcgen('''
>  
>      qmp_marshal_output_%(c_name)s(retval, ret, errp);
>  ''',
>                       c_name=ret_type.c_name())
> +
> +    if add_trace_events:
> +        if ret_type:
> +            ret += mcgen('''
> +
> +    if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
> +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
> +
> +        trace_qmp_exit_%(name)s(ret_json->str, true);
> +    }
> +    ''',
> +                         upper=upper, name=name)
> +        else:
> +            ret += mcgen('''
> +
> +    trace_qmp_exit_%(name)s("{}", true);
> +    ''',
> +                         name=name)
> +
>      return ret
>  
>  
> @@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
>                   proto=build_marshal_proto(name))
>  
>  
> +def gen_trace(name: str) -> str:
> +    return mcgen('''
> +qmp_enter_%(name)s(const char *json) "%%s"
> +qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
> +''',
> +                 name=c_name(name))
> +
> +
>  def gen_marshal(name: str,
>                  arg_type: Optional[QAPISchemaObjectType],
>                  boxed: bool,
> -                ret_type: Optional[QAPISchemaType]) -> str:
> +                ret_type: Optional[QAPISchemaType],
> +                add_trace_events: bool) -> str:
>      have_args = boxed or (arg_type and not arg_type.is_empty())
>      if have_args:
>          assert arg_type is not None
> @@ -180,7 +236,7 @@ def gen_marshal(name: str,
>      }
>  ''')
>  
> -    ret += gen_call(name, arg_type, boxed, ret_type)
> +    ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>  
>      ret += mcgen('''
>  
> @@ -238,11 +294,13 @@ def gen_register_command(name: str,
>  
>  
>  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> -    def __init__(self, prefix: str):
> +    def __init__(self, prefix: str, add_trace_events: bool):
>          super().__init__(
>              prefix, 'qapi-commands',
> -            ' * Schema-defined QAPI/QMP commands', None, __doc__)
> +            ' * Schema-defined QAPI/QMP commands', None, __doc__,
> +            add_trace_events=add_trace_events)
>          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> +        self.add_trace_events = add_trace_events
>  
>      def _begin_user_module(self, name: str) -> None:
>          self._visited_ret_types[self._genc] = set()
> @@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>  
>  ''',
>                               commands=commands, visit=visit))
> +
> +        if self.add_trace_events and commands != 'qapi-commands':
> +            self._genc.add(mcgen('''
> +#include "trace/trace-qapi.h"
> +#include "qapi/qmp/qjson.h"
> +#include "trace/trace-%(nm)s_trace_events.h"
> +''',
> +                                 nm=c_name(commands, protect=False)))
> +            # We use c_name(commands, protect=False) to turn '-' into '_', to
> +            # match .underscorify() in trace/meson.build
> +
>          self._genh.add(mcgen('''
>  #include "%(types)s.h"
>  
> @@ -322,7 +391,10 @@ def visit_command(self,
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>              self._genh.add(gen_marshal_decl(name))
> -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> +                                       self.add_trace_events))
> +            if self.add_trace_events:
> +                self._gent.add(gen_trace(name))
>          with self._temp_module('./init'):
>              with ifcontext(ifcond, self._genh, self._genc):
>                  self._genc.add(gen_register_command(
> @@ -332,7 +404,8 @@ def visit_command(self,
>  
>  def gen_commands(schema: QAPISchema,
>                   output_dir: str,
> -                 prefix: str) -> None:
> -    vis = QAPISchemaGenCommandVisitor(prefix)
> +                 prefix: str,
> +                 add_trace_events: bool) -> None:
> +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
>      schema.visit(vis)
>      vis.write(output_dir)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..2e61409f04 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -49,7 +49,7 @@ def generate(schema_file: str,
>      schema = QAPISchema(schema_file)
>      gen_types(schema, output_dir, prefix, builtins)
>      gen_visit(schema, output_dir, prefix, builtins)
> -    gen_commands(schema, output_dir, prefix)
> +    gen_commands(schema, output_dir, prefix, False)
>      gen_events(schema, output_dir, prefix)
>      gen_introspect(schema, output_dir, prefix, unmask)

Missing:

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index a3b5473089..feafed79b5 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1690,8 +1690,8 @@ Example::
         }
 
         retval = qmp_my_command(arg.arg1, &err);
-        error_propagate(errp, err);
         if (err) {
+            error_propagate(errp, err);
             goto out;
         }
 

Re: [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg

[Vladimir Sementsov-Ogievskiy]

25.01.2022 13:02, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> We are going to generate trace events for qmp commands. We should
>> generate both trace_*() function calls and trace-events files listing
>> events for trace generator.
>>
>> Now implement that possibility in gen_commands() function.
>>
>> The functionality will be used in the following commit, and now comment
>> in _begin_user_module() about c_name() is a bit premature.
> 
> Neglects to explain why it needs to be optional.
> 
> Suggest
> 
>    qapi/commands: Optionally generate trace for QMP commands
> 
>    Add trace generation disabled.  The next commit will enable it for
>    qapi/, but not for qga/ and tests/.  Making it work for the latter two
>    would involve some Meson hackery to ensure we generate the
>    trace-events files before trace-tool uses them.  Since we don't
>    actually support tracing there, we'll bypass that problem.
> 

Thanks, sounds good.

>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/qapi/commands.py | 101 +++++++++++++++++++++++++++++++++------
>>   scripts/qapi/main.py     |   2 +-
>>   2 files changed, 88 insertions(+), 15 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 21001bbd6b..166bf5dcbc 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -53,7 +53,8 @@ def gen_command_decl(name: str,
>>   def gen_call(name: str,
>>                arg_type: Optional[QAPISchemaObjectType],
>>                boxed: bool,
>> -             ret_type: Optional[QAPISchemaType]) -> str:
>> +             ret_type: Optional[QAPISchemaType],
>> +             add_trace_events: bool) -> str:
>>       ret = ''
>>   
>>       argstr = ''
>> @@ -71,21 +72,67 @@ def gen_call(name: str,
>>       if ret_type:
>>           lhs = 'retval = '
>>   
>> -    ret = mcgen('''
>> +    name = c_name(name)
>> +    upper = name.upper()
>>   
>> -    %(lhs)sqmp_%(c_name)s(%(args)s&err);
>> -    error_propagate(errp, err);
>> -''',
>> -                c_name=c_name(name), args=argstr, lhs=lhs)
>> -    if ret_type:
>> +    if add_trace_events:
>>           ret += mcgen('''
>> +
>> +    if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
>> +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
>> +
>> +        trace_qmp_enter_%(name)s(req_json->str);
>> +    }
>> +    ''',
>> +                     upper=upper, name=name)
>> +
>> +    ret += mcgen('''
>> +
>> +    %(lhs)sqmp_%(name)s(%(args)s&err);
>> +''',
>> +                 name=name, args=argstr, lhs=lhs)
>> +
>> +    ret += mcgen('''
>>       if (err) {
>> +''')
>> +
>> +    if add_trace_events:
>> +        ret += mcgen('''
>> +        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
>> +''',
>> +                     name=name)
>> +
>> +    ret += mcgen('''
>> +        error_propagate(errp, err);
>>           goto out;
>>       }
>> +''')
> 
> Before the patch, we generate
> 
>      error_propagate(errp, err);
> 
> and if there's any code between here and out:
> 
>      if (err) {
>          goto out;
>      }
> 
> After the patch, we always generate
> 
>      if (err) {
>          error_propagate(errp, err);
>          goto out;
>      }
> 
> But with trace generation enabled (next patch), it changes to
> 
>      if (err) {
>          trace_qmp_exit_FOO(error_get_pretty(err), false);
>          error_propagate(errp, err);
>          goto out;
>      }
> 
>      trace_qmp_exit_FOO("{}", true);
> 
> Okay.
> 
>> +
>> +    if ret_type:
>> +        ret += mcgen('''
>>   
>>       qmp_marshal_output_%(c_name)s(retval, ret, errp);
>>   ''',
>>                        c_name=ret_type.c_name())
>> +
>> +    if add_trace_events:
>> +        if ret_type:
>> +            ret += mcgen('''
>> +
>> +    if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
>> +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
>> +
>> +        trace_qmp_exit_%(name)s(ret_json->str, true);
>> +    }
>> +    ''',
>> +                         upper=upper, name=name)
>> +        else:
>> +            ret += mcgen('''
>> +
>> +    trace_qmp_exit_%(name)s("{}", true);
>> +    ''',
>> +                         name=name)
>> +
>>       return ret
>>   
>>   
>> @@ -122,10 +169,19 @@ def gen_marshal_decl(name: str) -> str:
>>                    proto=build_marshal_proto(name))
>>   
>>   
>> +def gen_trace(name: str) -> str:
>> +    return mcgen('''
>> +qmp_enter_%(name)s(const char *json) "%%s"
>> +qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
>> +''',
>> +                 name=c_name(name))
>> +
>> +
>>   def gen_marshal(name: str,
>>                   arg_type: Optional[QAPISchemaObjectType],
>>                   boxed: bool,
>> -                ret_type: Optional[QAPISchemaType]) -> str:
>> +                ret_type: Optional[QAPISchemaType],
>> +                add_trace_events: bool) -> str:
>>       have_args = boxed or (arg_type and not arg_type.is_empty())
>>       if have_args:
>>           assert arg_type is not None
>> @@ -180,7 +236,7 @@ def gen_marshal(name: str,
>>       }
>>   ''')
>>   
>> -    ret += gen_call(name, arg_type, boxed, ret_type)
>> +    ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>>   
>>       ret += mcgen('''
>>   
>> @@ -238,11 +294,13 @@ def gen_register_command(name: str,
>>   
>>   
>>   class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>> -    def __init__(self, prefix: str):
>> +    def __init__(self, prefix: str, add_trace_events: bool):
>>           super().__init__(
>>               prefix, 'qapi-commands',
>> -            ' * Schema-defined QAPI/QMP commands', None, __doc__)
>> +            ' * Schema-defined QAPI/QMP commands', None, __doc__,
>> +            add_trace_events=add_trace_events)
>>           self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
>> +        self.add_trace_events = add_trace_events
>>   
>>       def _begin_user_module(self, name: str) -> None:
>>           self._visited_ret_types[self._genc] = set()
>> @@ -261,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>>   
>>   ''',
>>                                commands=commands, visit=visit))
>> +
>> +        if self.add_trace_events and commands != 'qapi-commands':
>> +            self._genc.add(mcgen('''
>> +#include "trace/trace-qapi.h"
>> +#include "qapi/qmp/qjson.h"
>> +#include "trace/trace-%(nm)s_trace_events.h"
>> +''',
>> +                                 nm=c_name(commands, protect=False)))
>> +            # We use c_name(commands, protect=False) to turn '-' into '_', to
>> +            # match .underscorify() in trace/meson.build
>> +
>>           self._genh.add(mcgen('''
>>   #include "%(types)s.h"
>>   
>> @@ -322,7 +391,10 @@ def visit_command(self,
>>           with ifcontext(ifcond, self._genh, self._genc):
>>               self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>>               self._genh.add(gen_marshal_decl(name))
>> -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>> +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
>> +                                       self.add_trace_events))
>> +            if self.add_trace_events:
>> +                self._gent.add(gen_trace(name))
>>           with self._temp_module('./init'):
>>               with ifcontext(ifcond, self._genh, self._genc):
>>                   self._genc.add(gen_register_command(
>> @@ -332,7 +404,8 @@ def visit_command(self,
>>   
>>   def gen_commands(schema: QAPISchema,
>>                    output_dir: str,
>> -                 prefix: str) -> None:
>> -    vis = QAPISchemaGenCommandVisitor(prefix)
>> +                 prefix: str,
>> +                 add_trace_events: bool) -> None:
>> +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_events)
>>       schema.visit(vis)
>>       vis.write(output_dir)
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index f2ea6e0ce4..2e61409f04 100644
>> --- a/scripts/qapi/main.py
>> +++ b/scripts/qapi/main.py
>> @@ -49,7 +49,7 @@ def generate(schema_file: str,
>>       schema = QAPISchema(schema_file)
>>       gen_types(schema, output_dir, prefix, builtins)
>>       gen_visit(schema, output_dir, prefix, builtins)
>> -    gen_commands(schema, output_dir, prefix)
>> +    gen_commands(schema, output_dir, prefix, False)
>>       gen_events(schema, output_dir, prefix)
>>       gen_introspect(schema, output_dir, prefix, unmask)
> 
> Missing:
> 
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index a3b5473089..feafed79b5 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -1690,8 +1690,8 @@ Example::
>           }
>   
>           retval = qmp_my_command(arg.arg1, &err);
> -        error_propagate(errp, err);
>           if (err) {
> +            error_propagate(errp, err);
>               goto out;
>           }
>   
> 

Ah right. I fix doc in the next commit, but you are right, the behavior for disabled trace events is modified already in this commit.

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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

> 1. Add --no-trace-events to suppress trace events generation in some
>    cases, and make trace events be generated by default.
> 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>
> ---
>  docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--

The doc update isn't mentioned in the commit message.

>  meson.build                  |  3 +++
>  qapi/meson.build             |  7 +++++++
>  qga/meson.build              | 11 ++++++++++-
>  scripts/qapi/main.py         | 10 +++++++---
>  tests/meson.build            | 11 ++++++++++-
>  trace/meson.build            | 11 ++++++++---
>  7 files changed, 66 insertions(+), 10 deletions(-)

This commit consists of a small QAPI code generator change, build system
work to put it to use, and QAPI documentation update for the series'
feature.

I'd reshuffle as follows:

* Squash the main.py change into the previous commit.

* Split off the doc update into its own commit.

This way, build system experts can provide an R-by in good conscience
without reviewing the doc update, and vice versa.

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

[Vladimir Sementsov-Ogievskiy]

25.01.2022 13:25, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 1. Add --no-trace-events to suppress trace events generation in some
>>     cases, and make trace events be generated by default.
>> 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>
>> ---
>>   docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
> 
> The doc update isn't mentioned in the commit message.
> 
>>   meson.build                  |  3 +++
>>   qapi/meson.build             |  7 +++++++
>>   qga/meson.build              | 11 ++++++++++-
>>   scripts/qapi/main.py         | 10 +++++++---
>>   tests/meson.build            | 11 ++++++++++-
>>   trace/meson.build            | 11 ++++++++---
>>   7 files changed, 66 insertions(+), 10 deletions(-)
> 
> This commit consists of a small QAPI code generator change, build system
> work to put it to use, and QAPI documentation update for the series'
> feature.
> 
> I'd reshuffle as follows:
> 
> * Squash the main.py change into the previous commit.
> 
> * Split off the doc update into its own commit.
> 
> This way, build system experts can provide an R-by in good conscience
> without reviewing the doc update, and vice versa.
> 

But I think this way build will fail on previous commit. Or we should still keep trace-generation disabled in previous commit, and enable it only together with meson changes.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

25.01.2022 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 25.01.2022 13:25, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 1. Add --no-trace-events to suppress trace events generation in some
>>>     cases, and make trace events be generated by default.
>>> 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>
>>> ---
>>>   docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
>>
>> The doc update isn't mentioned in the commit message.
>>
>>>   meson.build                  |  3 +++
>>>   qapi/meson.build             |  7 +++++++
>>>   qga/meson.build              | 11 ++++++++++-
>>>   scripts/qapi/main.py         | 10 +++++++---
>>>   tests/meson.build            | 11 ++++++++++-
>>>   trace/meson.build            | 11 ++++++++---
>>>   7 files changed, 66 insertions(+), 10 deletions(-)
>>
>> This commit consists of a small QAPI code generator change, build system
>> work to put it to use, and QAPI documentation update for the series'
>> feature.
>>
>> I'd reshuffle as follows:
>>
>> * Squash the main.py change into the previous commit.
>>
>> * Split off the doc update into its own commit.
>>
>> This way, build system experts can provide an R-by in good conscience
>> without reviewing the doc update, and vice versa.
>>
> 
> But I think this way build will fail on previous commit. Or we should still keep trace-generation disabled in previous commit, and enable it only together with meson changes.
> 

May be keep positive option --gen-trace-events in previous commit, like in my previous version? This way meson-changing commit becomes self-sufficient. And then in additional commit change the default and drop --gen-trace-events option and add --no-trace-events instead.

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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

> 25.01.2022 14:03, Vladimir Sementsov-Ogievskiy wrote:
>> 25.01.2022 13:25, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 1. Add --no-trace-events to suppress trace events generation in some
>>>>     cases, and make trace events be generated by default.
>>>> 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>
>>>> ---
>>>>   docs/devel/qapi-code-gen.rst | 23 +++++++++++++++++++++--
>>>
>>> The doc update isn't mentioned in the commit message.
>>>
>>>>   meson.build                  |  3 +++
>>>>   qapi/meson.build             |  7 +++++++
>>>>   qga/meson.build              | 11 ++++++++++-
>>>>   scripts/qapi/main.py         | 10 +++++++---
>>>>   tests/meson.build            | 11 ++++++++++-
>>>>   trace/meson.build            | 11 ++++++++---
>>>>   7 files changed, 66 insertions(+), 10 deletions(-)
>>>
>>> This commit consists of a small QAPI code generator change, build system
>>> work to put it to use, and QAPI documentation update for the series'
>>> feature.
>>>
>>> I'd reshuffle as follows:
>>>
>>> * Squash the main.py change into the previous commit.
>>>
>>> * Split off the doc update into its own commit.
>>>
>>> This way, build system experts can provide an R-by in good conscience
>>> without reviewing the doc update, and vice versa.
>>>
>> But I think this way build will fail on previous commit. Or we
>> should still keep trace-generation disabled in previous commit, and
>> enable it only together with meson changes.
>> 
>
> May be keep positive option --gen-trace-events in previous commit, like in my previous version? This way meson-changing commit becomes self-sufficient. And then in additional commit change the default and drop --gen-trace-events option and add --no-trace-events instead.

You choose.  But I'd spell it --gen-trace.

Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[Markus Armbruster]

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

> 25.01.2022 12:07, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> We are going to generate trace events for qmp commands. We should
>> 
>> QMP
>> 
>>> generate both trace_*() function calls and trace-events files listing
>>> events for trace generator.
>>>
>>> So, add an output module FOO.trace-events for each FOO schema module.
>>>
>>> Still, we'll need these .trace-events files only for
>>> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
>>> So, make this possibility optional, to avoid generating extra empty
>>> files for all other successors of QAPISchemaModularCVisitor.
>> 
>> Suggest to make this slightly less technical for easier reading:
>> 
>>    Since we're going to add trace events only to command marshallers,
>>    make the trace-events output optional, so we don't generate so many
>>    useless empty files.
>
> Sounds good
>
>> 
>>> We can't simply add the new feature directly to
>>> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
>>> a kind of ._module / .write() functionality of parent class in the
>>> successor, which seems worse than extending base class functionality.
>> 
>> Do you mean something like
>> 
>>    The alternative would be adding the the new feature directly to
>>    QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
>>    ._module / .write() functionality of its parent class
>>    QAPISchemaModularCVisitor, which seems worse than extending the parent
>>    class.
>> 
>> ?
>
> Yes.
>
>> 
>> If yes: I'm not sure about "worse". 
>
> Hmm. *shrug* ) I'm new to this code, that's why it seems unobvious to me, and that's why I'm afraid of deeper refactoring)
>
>> But keeping it in the parent class
>> feels right to me anyway, as trace events could be useful in other child
>> classes, too.
>
> If it is OK, we can simply drop this paragraph.

Works for me.

Keeping it would work, too.  "Seems worse" is an opinion, not wrong.

>>> Currently nobody set add_trace_events to True, so new functionality is
>>> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
>> 
>> Drop "formally".
>> 
>>> in further commit.
>> 
>> "in a further commit", or "in the next commit".
>> 
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

[...]

>> I wonder whether we really need a new __init__() parameter.  Could we
>> have ._gent() create the module on demand?  This is *not* a demand.
>> 
>
> My first attempt to drop extra empty generated .trace-events files was to teach QAPIGenTrace() not to generate file when it is empty. But in this case some empty .trace-events for commands are not generated, and "include" line fails to compile. And at time when include line is generated, I don't know will corresponding .trace-events be empty or not. So I decided to make a new parameter for __init__()

Okay.  We can always improve on top if we care.