[PATCH v5 0/7] trace qmp commands

[PATCH v5 0/7] trace qmp commands

[Vladimir Sementsov-Ogievskiy]

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

v5: small fixes and rewordings, + reshuffle patches so that main meson change now is like in v3 and Paolo's a-b make sense again.

01: - fix/reword commit message
    - fix typing in qapi/gen.py
    - rename add_trace_events to gen_trace_events, and to _gen_trace_event for private attribute
02: - split from 03, to make 03 a bit simpler
03: - reword commit message
    - rename add_trace_events to gen_trace_events, and to _gen_trace_event for private attribute
    - rebase on 02
    - merge here main.py changes, with new option: --gen-trace
04: - move some parts to other commits, so now it looks like 03 patch of v3, so add back a-b mark by Paolo
05: - split doc change from 04
06: - split new comments from 04
07: new

Vladimir Sementsov-Ogievskiy (7):
  scripts/qapi/gen.py: add FOO.trace-events output module
  qapi/commands: refactor error handling code
  qapi/commands: Optionally generate trace for QMP commands
  meson: generate trace events for qmp commands
  docs/qapi-code-gen: update to cover trace events code generation
  meson: document, why we don't generate trace events for tests/ and
    qga/
  qapi: generate trace events by default

 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          |  33 ++++++++++--
 scripts/qapi/main.py         |  10 ++--
 tests/meson.build            |  11 +++-
 trace/meson.build            |  11 ++--
 9 files changed, 182 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH v5 1/7] 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.

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.

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

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

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 995a97d2b8..a41a2c1d55 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,
+                 gen_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._gen_trace_events = gen_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._gen_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,14 @@ 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._gen_trace_events:
+            gent = QAPIGenTrace(basename + '.trace-events')
+        else:
+            gent = None
+
+        self._module[name] = (genc, genh, gent)
         self._current_module = name
 
     @contextmanager
@@ -304,11 +327,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 v5 2/7] qapi/commands: refactor error handling code

[Vladimir Sementsov-Ogievskiy]

Move error_propagate() to if (err) and make "if (err)" block mandatory.
This is to simplify further commit, which will bring trace events
generation for QMP commands.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/devel/qapi-code-gen.rst |  2 +-
 scripts/qapi/commands.py     | 10 +++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

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;
         }
 
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21001bbd6b..17e5ed2414 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -74,14 +74,18 @@ def gen_call(name: str,
     ret = mcgen('''
 
     %(lhs)sqmp_%(c_name)s(%(args)s&err);
-    error_propagate(errp, err);
 ''',
                 c_name=c_name(name), args=argstr, lhs=lhs)
-    if ret_type:
-        ret += mcgen('''
+
+    ret += mcgen('''
     if (err) {
+        error_propagate(errp, err);
         goto out;
     }
+''')
+
+    if ret_type:
+        ret += mcgen('''
 
     qmp_marshal_output_%(c_name)s(retval, ret, errp);
 ''',
-- 
2.31.1

[PATCH v5 3/7] qapi/commands: Optionally generate trace for QMP commands

[Vladimir Sementsov-Ogievskiy]

Add trace generation disabled by default and new option --gen-trace to
enable it.  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 | 91 +++++++++++++++++++++++++++++++++++-----
 scripts/qapi/main.py     | 10 +++--
 2 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 17e5ed2414..0c171cb880 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],
+             gen_trace_events: bool) -> str:
     ret = ''
 
     argstr = ''
@@ -71,14 +72,37 @@ 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);
+    if gen_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);
 ''',
-                c_name=c_name(name), args=argstr, lhs=lhs)
+                 name=name, args=argstr, lhs=lhs)
 
     ret += mcgen('''
     if (err) {
+''')
+
+    if gen_trace_events:
+        ret += mcgen('''
+        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
+''',
+                     name=name)
+
+    ret += mcgen('''
         error_propagate(errp, err);
         goto out;
     }
@@ -90,6 +114,25 @@ def gen_call(name: str,
     qmp_marshal_output_%(c_name)s(retval, ret, errp);
 ''',
                      c_name=ret_type.c_name())
+
+    if gen_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
 
 
@@ -126,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],
+                gen_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
@@ -184,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, gen_trace_events)
 
     ret += mcgen('''
 
@@ -242,11 +294,13 @@ def gen_register_command(name: str,
 
 
 class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
-    def __init__(self, prefix: str):
+    def __init__(self, prefix: str, gen_trace_events: bool):
         super().__init__(
             prefix, 'qapi-commands',
-            ' * Schema-defined QAPI/QMP commands', None, __doc__)
+            ' * Schema-defined QAPI/QMP commands', None, __doc__,
+            gen_trace_events=gen_trace_events)
         self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
+        self._gen_trace_events = gen_trace_events
 
     def _begin_user_module(self, name: str) -> None:
         self._visited_ret_types[self._genc] = set()
@@ -265,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
 
 ''',
                              commands=commands, visit=visit))
+
+        if self._gen_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"
 
@@ -326,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._gen_trace_events))
+            if self._gen_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(
@@ -336,7 +404,8 @@ def visit_command(self,
 
 def gen_commands(schema: QAPISchema,
                  output_dir: str,
-                 prefix: str) -> None:
-    vis = QAPISchemaGenCommandVisitor(prefix)
+                 prefix: str,
+                 gen_trace_events: bool) -> None:
+    vis = QAPISchemaGenCommandVisitor(prefix, gen_trace_events)
     schema.visit(vis)
     vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..ecff41910c 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,
+             gen_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, gen_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('--gen-trace', 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,
+                 gen_trace_events=args.gen_trace)
     except QAPIError as err:
         print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
         return 1
-- 
2.31.1

[PATCH v5 4/7] 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>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build       |  3 +++
 qapi/meson.build  |  9 ++++++++-
 trace/meson.build | 11 ++++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

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..b22558ca73 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@', '--gen-trace' ],
   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

[PATCH v5 5/7] docs/qapi-code-gen: update to cover trace events code generation

[Vladimir Sementsov-Ogievskiy]

Previous commits enabled trace events generation for most of QAPI
generated code (except for tests/ and qga/). Let's update documentation
to illustrate it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/devel/qapi-code-gen.rst | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index feafed79b5..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);
         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();
-- 
2.31.1

[PATCH v5 6/7] meson: document, why we don't generate trace events for tests/ and qga/

[Vladimir Sementsov-Ogievskiy]

Making trace generation work for tests/ and qga/ 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
anyway, we bypass that problem.

Let's add corresponding comments.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qga/meson.build   | 8 ++++++++
 tests/meson.build | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/qga/meson.build b/qga/meson.build
index cfb1fbc085..79fcf91751 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -15,6 +15,14 @@ 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',
diff --git a/tests/meson.build b/tests/meson.build
index 3f3882748a..21857d8b01 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -31,6 +31,14 @@ 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',
-- 
2.31.1

[PATCH v5 7/7] qapi: generate trace events by default

[Vladimir Sementsov-Ogievskiy]

We don't generate trace events for tests/ and qga/ because that it is
not simple and not necessary. We have corresponding comments in both
tests/meson.build and qga/meson.build.

Still to not miss possible future qapi code generation call, and not to
forget to enable trace events generation, let's enable it by default.
So, turn option --gen-trace into opposite --no-trace-events and use new
option only in tests/ and qga/ where we already have good comments why
we don't generate trace events code.

Note that this commit enables trace-events generation for qapi-gen.py
call from tests/qapi-schema/meson.build and storage-daemon/meson.build.
Still, both are kind of noop: tests/qapi-schema/ doesn't seem to
generate any QMP command code and no .trace-events files anyway,
storage-daemon/ uses common QMP command implementations and just
generate empty .trace-events

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/meson.build     | 2 +-
 qga/meson.build      | 3 ++-
 scripts/qapi/main.py | 6 +++---
 tests/meson.build    | 3 ++-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/qapi/meson.build b/qapi/meson.build
index b22558ca73..656ef0e039 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -127,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@', '--gen-trace' ],
+  command: [ qapi_gen, '-o', 'qapi', '-b', '@INPUT0@' ],
   depend_files: [ qapi_inputs, qapi_gen_depends ])
 
 # Now go through all the outputs and add them to the right sourceset.
diff --git a/qga/meson.build b/qga/meson.build
index 79fcf91751..1f2818a1b9 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -26,7 +26,8 @@ qga_qapi_outputs = [
 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 ecff41910c..ca1737aa2b 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -75,8 +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('--gen-trace', action='store_true',
-                        help="add trace events to qmp marshals")
+    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()
 
@@ -92,7 +92,7 @@ def main() -> int:
                  prefix=args.prefix,
                  unmask=args.unmask,
                  builtins=args.builtins,
-                 gen_trace_events=args.gen_trace)
+                 gen_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 21857d8b01..b4b95cc198 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -45,7 +45,8 @@ test_qapi_files = custom_target('Test QAPI files',
                                              '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
-- 
2.31.1

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Tue, Jan 25, 2022 at 10:56:49PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> @@ -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:

If you respin maybe rename this to _gentrace() or even
_gen_trace_events() so the name is clearer (although the latter collides
with the self._gen_trace_events field and may need to be renamed to
enable_trace_events or similar).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 6/7] meson: document, why we don't generate trace events for tests/ and qga/

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

On Tue, Jan 25, 2022 at 10:56:54PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Making trace generation work for tests/ and qga/ 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
> anyway, we bypass that problem.
> 
> Let's add corresponding comments.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qga/meson.build   | 8 ++++++++
>  tests/meson.build | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/qga/meson.build b/qga/meson.build
> index cfb1fbc085..79fcf91751 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -15,6 +15,14 @@ qga_qapi_outputs = [
>    'qga-qapi-visit.h',
>  ]
>  
> +# We don't generate trace-events, just because it's not simple. For do it,

s/For/To/

> +# we also should add .trace-events file into qga_qapi_outputs, and than

s/than/then/

> +# add corresponding element of qga_qapi_files into qapi_trace_events
> +# global list, which is processed than in trace/meson.build.

s/processed than/then processed/

> +# 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',
> diff --git a/tests/meson.build b/tests/meson.build
> index 3f3882748a..21857d8b01 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -31,6 +31,14 @@ 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.

Similar changes as those above.

I can do it when merging the patches.

>  test_qapi_files = custom_target('Test QAPI files',
>                                  output: test_qapi_outputs,
>                                  input: files('qapi-schema/qapi-schema-test.json',
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 6/7] meson: document, why we don't generate trace events for tests/ and qga/

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

Come to think of it, this is more of a QAPI code generator series so
Markus is probably the one merging, not me. Sorry for the confusion :).

Markus: Please see the small fixups I've suggested. They could be
applied when merging this series.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 0/7] trace qmp commands

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

On Tue, Jan 25, 2022 at 10:56:48PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> This series aims to add trace points for each qmp command with help of
> qapi code generator.
> 
> v5: small fixes and rewordings, + reshuffle patches so that main meson change now is like in v3 and Paolo's a-b make sense again.
> 
> 01: - fix/reword commit message
>     - fix typing in qapi/gen.py
>     - rename add_trace_events to gen_trace_events, and to _gen_trace_event for private attribute
> 02: - split from 03, to make 03 a bit simpler
> 03: - reword commit message
>     - rename add_trace_events to gen_trace_events, and to _gen_trace_event for private attribute
>     - rebase on 02
>     - merge here main.py changes, with new option: --gen-trace
> 04: - move some parts to other commits, so now it looks like 03 patch of v3, so add back a-b mark by Paolo
> 05: - split doc change from 04
> 06: - split new comments from 04
> 07: new
> 
> Vladimir Sementsov-Ogievskiy (7):
>   scripts/qapi/gen.py: add FOO.trace-events output module
>   qapi/commands: refactor error handling code
>   qapi/commands: Optionally generate trace for QMP commands
>   meson: generate trace events for qmp commands
>   docs/qapi-code-gen: update to cover trace events code generation
>   meson: document, why we don't generate trace events for tests/ and
>     qga/
>   qapi: generate trace events by default
> 
>  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          |  33 ++++++++++--
>  scripts/qapi/main.py         |  10 ++--
>  tests/meson.build            |  11 +++-
>  trace/meson.build            |  11 ++--
>  9 files changed, 182 insertions(+), 28 deletions(-)
> 
> -- 
> 2.31.1
> 

Thanks, Vladimir. This looks great!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Markus Armbruster]

Let's tweak the title to more closely match existing commits:

    qapi/gen: Add FOO.trace-events output module

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.
>
> So, add an output module FOO.trace-events for each FOO schema module.
>
> 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.
>
> Currently nobody set add_trace_events to True, so new functionality is
> disabled. It will be enabled for QAPISchemaGenCommandVisitor
> in a further commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/gen.py | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..a41a2c1d55 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,
> +                 gen_trace_events: bool = False):

Let's rename to @gen_trace for consistency with PATCH 3's --gen-trace.

Hmm, PATCH 7 replaces it by --no-trace-events.  I'm going to suggest
--suppress-tracing there.  @gen_tracing?

>          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._gen_trace_events = gen_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._gen_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,14 @@ 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._gen_trace_events:
> +            gent = QAPIGenTrace(basename + '.trace-events')
> +        else:
> +            gent = None

A bit more compact:

           gent: Optional[QAPIGenTrace] = None
           if self._gen_trace_events:
               gent = QAPIGenTrace(basename + '.trace-events')

> +
> +        self._module[name] = (genc, genh, gent)
>          self._current_module = name
>  
>      @contextmanager
> @@ -304,11 +327,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

Re: [PATCH v5 3/7] qapi/commands: Optionally generate trace for QMP commands

[Markus Armbruster]

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

> Add trace generation disabled by default and new option --gen-trace to
> enable it.  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 | 91 +++++++++++++++++++++++++++++++++++-----
>  scripts/qapi/main.py     | 10 +++--
>  2 files changed, 87 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 17e5ed2414..0c171cb880 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],
> +             gen_trace_events: bool) -> str:
>      ret = ''
>  
>      argstr = ''
> @@ -71,14 +72,37 @@ 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);
> +    if gen_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);
>  ''',
> -                c_name=c_name(name), args=argstr, lhs=lhs)
> +                 name=name, args=argstr, lhs=lhs)
>  
>      ret += mcgen('''
>      if (err) {
> +''')
> +
> +    if gen_trace_events:
> +        ret += mcgen('''
> +        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
> +''',
> +                     name=name)
> +
> +    ret += mcgen('''
>          error_propagate(errp, err);
>          goto out;
>      }
> @@ -90,6 +114,25 @@ def gen_call(name: str,
>      qmp_marshal_output_%(c_name)s(retval, ret, errp);
>  ''',
>                       c_name=ret_type.c_name())
> +
> +    if gen_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
>  
>  
> @@ -126,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],
> +                gen_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
> @@ -184,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, gen_trace_events)
>  
>      ret += mcgen('''
>  
> @@ -242,11 +294,13 @@ def gen_register_command(name: str,
>  
>  
>  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> -    def __init__(self, prefix: str):
> +    def __init__(self, prefix: str, gen_trace_events: bool):
>          super().__init__(
>              prefix, 'qapi-commands',
> -            ' * Schema-defined QAPI/QMP commands', None, __doc__)
> +            ' * Schema-defined QAPI/QMP commands', None, __doc__,
> +            gen_trace_events=gen_trace_events)
>          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> +        self._gen_trace_events = gen_trace_events
>  
>      def _begin_user_module(self, name: str) -> None:
>          self._visited_ret_types[self._genc] = set()
> @@ -265,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>  
>  ''',
>                               commands=commands, visit=visit))
> +
> +        if self._gen_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"
>  
> @@ -326,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._gen_trace_events))
> +            if self._gen_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(
> @@ -336,7 +404,8 @@ def visit_command(self,
>  
>  def gen_commands(schema: QAPISchema,
>                   output_dir: str,
> -                 prefix: str) -> None:
> -    vis = QAPISchemaGenCommandVisitor(prefix)
> +                 prefix: str,
> +                 gen_trace_events: bool) -> None:
> +    vis = QAPISchemaGenCommandVisitor(prefix, gen_trace_events)
>      schema.visit(vis)
>      vis.write(output_dir)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..ecff41910c 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,
> +             gen_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, gen_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")

Let's add

       # Option --gen-trace exists so we can avoid solving build system
       # problems.  TODO Drop it when we no longer need it.

> +    parser.add_argument('--gen-trace', 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,
> +                 gen_trace_events=args.gen_trace)
>      except QAPIError as err:
>          print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
>          return 1

Re: [PATCH v5 6/7] meson: document, why we don't generate trace events for tests/ and qga/

[Markus Armbruster]

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

> Making trace generation work for tests/ and qga/ 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
> anyway, we bypass that problem.
>
> Let's add corresponding comments.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qga/meson.build   | 8 ++++++++
>  tests/meson.build | 8 ++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/qga/meson.build b/qga/meson.build
> index cfb1fbc085..79fcf91751 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -15,6 +15,14 @@ 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.

The option is still called --gen-trace at this point.  Easy fix: squash
into the next commit.

Suggest

   # Problem: to generate trace events, we'd have to add the .trace-events
   # file to qapi_trace_events like we do in qapi/meson.build.  Since
   # qapi_trace_events is used by trace/meson.build, we'd have to move
   # subdir('qga') above subdir('trace') in the top-level meson.build.
   # Can't, because it would break the dependency of qga on qemuutil (which
   # depends on trace_ss).  Not worth solving now; simply suppress trace
   # event generation instead.

>  qga_qapi_files = custom_target('QGA QAPI files',
>                                 output: qga_qapi_outputs,
>                                 input: 'qapi-schema.json',
> diff --git a/tests/meson.build b/tests/meson.build
> index 3f3882748a..21857d8b01 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -31,6 +31,14 @@ 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',

Likewise.

Re: [PATCH v5 7/7] qapi: generate trace events by default

[Markus Armbruster]

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

> We don't generate trace events for tests/ and qga/ because that it is
> not simple and not necessary. We have corresponding comments in both
> tests/meson.build and qga/meson.build.
>
> Still to not miss possible future qapi code generation call, and not to
> forget to enable trace events generation, let's enable it by default.
> So, turn option --gen-trace into opposite --no-trace-events and use new

Let's call it --suppress-tracing.

> option only in tests/ and qga/ where we already have good comments why
> we don't generate trace events code.
>
> Note that this commit enables trace-events generation for qapi-gen.py
> call from tests/qapi-schema/meson.build and storage-daemon/meson.build.
> Still, both are kind of noop: tests/qapi-schema/ doesn't seem to
> generate any QMP command code and no .trace-events files anyway,
> storage-daemon/ uses common QMP command implementations and just
> generate empty .trace-events
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Re: [PATCH v5 5/7] docs/qapi-code-gen: update to cover trace events code generation

[Markus Armbruster]

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

> Previous commits enabled trace events generation for most of QAPI
> generated code (except for tests/ and qga/). Let's update documentation
> to illustrate it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/devel/qapi-code-gen.rst | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index feafed79b5..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>`.

I think references look like :ref:`tracing`.

The last sentence is kind of redundant with the text added in the next
hunk.  Drop both new sentences?

>  
>  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>`.

Suggest

        Trace event declarations, see :ref:`tracing`.

> +
>   ``$(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);
>          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();

Let's add

       $ cat qapi-generated/example-qapi-commands.trace-events
       # AUTOMATICALLY GENERATED, DO NOT MODIFY

       qmp_enter_my_command(const char *json) "%s"
       qmp_exit_my_command(const char *result, bool succeeded) "%s %d"

between .h and .c.

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Tue, Jan 25, 2022 at 10:56:49PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> @@ -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:
>
> If you respin maybe rename this to _gentrace() or even
> _gen_trace_events() so the name is clearer (although the latter collides
> with the self._gen_trace_events field and may need to be renamed to
> enable_trace_events or similar).

We have ._genc() for .c, and ._genh() for .h.  Applying the same to
.trace-events results in ._gentrace_events, but that's ugly.

I'm okay with ._gen_trace_events().  Regarding the name collision: I
already suggested renaming attribute ._gen_trace_events to
._gen_tracing.

We might want to rename ._genc() and .genh() to .gen_c() and .gen_h()
for consistency.  Don't worry about that now.

Re: [PATCH v5 0/7] trace qmp commands

[Markus Armbruster]

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

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

I found only really minor things.  A quick respin should get us to the
finish line.  Thanks!