[PATCH v2 0/4] trace qmp commands

[PATCH v2 0/4] 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.

v2:
01: new
02: use qmp_* naming for new trace-events
03: add Philippe's r-b, thanks!
04: rewrite, so that it works now! Thanks to Paolo for fast help!

Vladimir Sementsov-Ogievskiy (4):
  jobs: drop qmp_ trace points
  scripts/qapi/commands: gen_commands(): add add_trace_points argument
  scripts/qapi-gen.py: add --add-trace-points option
  meson: generate trace points for qmp commands

 meson.build              |  1 +
 blockdev.c               |  8 ----
 job-qmp.c                |  6 ---
 block/trace-events       |  9 -----
 qapi/meson.build         |  9 ++++-
 scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
 scripts/qapi/gen.py      | 13 +++++--
 scripts/qapi/main.py     | 10 +++--
 trace-events             |  8 ----
 trace/meson.build        | 11 ++++--
 10 files changed, 107 insertions(+), 52 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] jobs: drop qmp_ trace points

[Vladimir Sementsov-Ogievskiy]

We are going to implement automatic trace points for qmp commands.
These several trace points are in conflict with upcoming ones. So, drop
them now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c         | 8 --------
 job-qmp.c          | 6 ------
 block/trace-events | 9 ---------
 trace-events       | 8 --------
 4 files changed, 31 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0eb2823b1b..10961d81a4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2586,8 +2586,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
-    trace_qmp_block_stream(bs);
-
 out:
     aio_context_release(aio_context);
 }
@@ -3354,7 +3352,6 @@ void qmp_block_job_cancel(const char *device,
         goto out;
     }
 
-    trace_qmp_block_job_cancel(job);
     job_user_cancel(&job->job, force, errp);
 out:
     aio_context_release(aio_context);
@@ -3369,7 +3366,6 @@ void qmp_block_job_pause(const char *device, Error **errp)
         return;
     }
 
-    trace_qmp_block_job_pause(job);
     job_user_pause(&job->job, errp);
     aio_context_release(aio_context);
 }
@@ -3383,7 +3379,6 @@ void qmp_block_job_resume(const char *device, Error **errp)
         return;
     }
 
-    trace_qmp_block_job_resume(job);
     job_user_resume(&job->job, errp);
     aio_context_release(aio_context);
 }
@@ -3397,7 +3392,6 @@ void qmp_block_job_complete(const char *device, Error **errp)
         return;
     }
 
-    trace_qmp_block_job_complete(job);
     job_complete(&job->job, errp);
     aio_context_release(aio_context);
 }
@@ -3411,7 +3405,6 @@ void qmp_block_job_finalize(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_block_job_finalize(job);
     job_ref(&job->job);
     job_finalize(&job->job, errp);
 
@@ -3435,7 +3428,6 @@ void qmp_block_job_dismiss(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_block_job_dismiss(bjob);
     job = &bjob->job;
     job_dismiss(&job, errp);
     aio_context_release(aio_context);
diff --git a/job-qmp.c b/job-qmp.c
index 829a28aa70..cf0cb9d717 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -57,7 +57,6 @@ void qmp_job_cancel(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_cancel(job);
     job_user_cancel(job, true, errp);
     aio_context_release(aio_context);
 }
@@ -71,7 +70,6 @@ void qmp_job_pause(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_pause(job);
     job_user_pause(job, errp);
     aio_context_release(aio_context);
 }
@@ -85,7 +83,6 @@ void qmp_job_resume(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_resume(job);
     job_user_resume(job, errp);
     aio_context_release(aio_context);
 }
@@ -99,7 +96,6 @@ void qmp_job_complete(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_complete(job);
     job_complete(job, errp);
     aio_context_release(aio_context);
 }
@@ -113,7 +109,6 @@ void qmp_job_finalize(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_finalize(job);
     job_ref(job);
     job_finalize(job, errp);
 
@@ -136,7 +131,6 @@ void qmp_job_dismiss(const char *id, Error **errp)
         return;
     }
 
-    trace_qmp_job_dismiss(job);
     job_dismiss(&job, errp);
     aio_context_release(aio_context);
 }
diff --git a/block/trace-events b/block/trace-events
index 549090d453..5be3e3913b 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -49,15 +49,6 @@ block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64"
 block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
 block_copy_write_zeroes_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
 
-# ../blockdev.c
-qmp_block_job_cancel(void *job) "job %p"
-qmp_block_job_pause(void *job) "job %p"
-qmp_block_job_resume(void *job) "job %p"
-qmp_block_job_complete(void *job) "job %p"
-qmp_block_job_finalize(void *job) "job %p"
-qmp_block_job_dismiss(void *job) "job %p"
-qmp_block_stream(void *bs) "bs %p"
-
 # file-win32.c
 file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d"
 
diff --git a/trace-events b/trace-events
index a637a61eba..1265f1e0cc 100644
--- a/trace-events
+++ b/trace-events
@@ -79,14 +79,6 @@ job_state_transition(void *job,  int ret, const char *legal, const char *s0, con
 job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
 job_completed(void *job, int ret) "job %p ret %d"
 
-# job-qmp.c
-qmp_job_cancel(void *job) "job %p"
-qmp_job_pause(void *job) "job %p"
-qmp_job_resume(void *job) "job %p"
-qmp_job_complete(void *job) "job %p"
-qmp_job_finalize(void *job) "job %p"
-qmp_job_dismiss(void *job) "job %p"
-
 
 ### Guest events, keep at bottom
 
-- 
2.31.1

[PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Vladimir Sementsov-Ogievskiy]

Add possibility to generate trace points for each qmp command.

We should generate both trace points and trace-events file, for further
trace point code generation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21001bbd6b..9691c11f96 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_points: bool) -> str:
     ret = ''
 
     argstr = ''
@@ -71,21 +72,65 @@ def gen_call(name: str,
     if ret_type:
         lhs = 'retval = '
 
-    ret = mcgen('''
+    qmp_name = f'qmp_{c_name(name)}'
+    upper = qmp_name.upper()
+
+    if add_trace_points:
+        ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
+        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+        trace_%(qmp_name)s("", req_json->str);
+    }
+    ''',
+                     upper=upper, qmp_name=qmp_name)
+
+    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) {
+''')
+
+    if add_trace_points:
+        ret += mcgen('''
+        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
+''',
+                     qmp_name=qmp_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_points:
+        if ret_type:
+            ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
+        g_autoptr(GString) ret_json = qobject_to_json(*ret);
+        trace_%(qmp_name)s("RET:", ret_json->str);
+    }
+    ''',
+                     upper=upper, qmp_name=qmp_name)
+        else:
+            ret += mcgen('''
+
+    trace_%(qmp_name)s("SUCCESS", "");
+    ''',
+                         qmp_name=qmp_name)
+
     return ret
 
 
@@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
                  proto=build_marshal_proto(name))
 
 
+def gen_trace(name: str) -> str:
+    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
+
 def gen_marshal(name: str,
                 arg_type: Optional[QAPISchemaObjectType],
                 boxed: bool,
-                ret_type: Optional[QAPISchemaType]) -> str:
+                ret_type: Optional[QAPISchemaType],
+                add_trace_points: 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 +229,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_points)
 
     ret += mcgen('''
 
@@ -238,11 +287,12 @@ def gen_register_command(name: str,
 
 
 class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
-    def __init__(self, prefix: str):
+    def __init__(self, prefix: str, add_trace_points: bool):
         super().__init__(
             prefix, 'qapi-commands',
             ' * Schema-defined QAPI/QMP commands', None, __doc__)
         self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
+        self.add_trace_points = add_trace_points
 
     def _begin_user_module(self, name: str) -> None:
         self._visited_ret_types[self._genc] = set()
@@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
 
 ''',
                              commands=commands, visit=visit))
+
+        if self.add_trace_points and c_name(commands) != 'qapi_commands':
+            self._genc.add(mcgen('''
+#include "trace/trace-qapi.h"
+#include "qapi/qmp/qjson.h"
+#include "trace/trace-%(nm)s_trace_events.h"
+''',
+                                 nm=c_name(commands)))
+
         self._genh.add(mcgen('''
 #include "%(types)s.h"
 
@@ -322,7 +381,9 @@ def visit_command(self,
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
             self._genh.add(gen_marshal_decl(name))
-            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
+                                       self.add_trace_points))
+            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 +393,8 @@ def visit_command(self,
 
 def gen_commands(schema: QAPISchema,
                  output_dir: str,
-                 prefix: str) -> None:
-    vis = QAPISchemaGenCommandVisitor(prefix)
+                 prefix: str,
+                 add_trace_points: bool) -> None:
+    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
     schema.visit(vis)
     vis.write(output_dir)
-- 
2.31.1

[PATCH v2 3/4] scripts/qapi-gen.py: add --add-trace-points option

[Vladimir Sementsov-Ogievskiy]

Add and option to generate trace points. We should generate both trace
points and trace-events files for further trace point code generation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 scripts/qapi/gen.py  | 13 ++++++++++---
 scripts/qapi/main.py | 10 +++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 995a97d2b8..605b3fe68a 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -251,7 +251,7 @@ def __init__(self,
         self._builtin_blurb = builtin_blurb
         self._pydoc = pydoc
         self._current_module: Optional[str] = None
-        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
+        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
         self._main_module: Optional[str] = None
 
     @property
@@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
         assert self._current_module is not None
         return self._module[self._current_module][1]
 
+    @property
+    def _gent(self) -> QAPIGen:
+        assert self._current_module is not None
+        return self._module[self._current_module][2]
+
     @staticmethod
     def _module_dirname(name: str) -> str:
         if QAPISchemaModule.is_user_module(name):
@@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
         basename = self._module_filename(self._what, name)
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
-        self._module[name] = (genc, genh)
+        gent = QAPIGen(basename + '.trace-events')
+        self._module[name] = (genc, genh, gent)
         self._current_module = name
 
     @contextmanager
@@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
         self._current_module = old_module
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
-        for name, (genc, genh) in self._module.items():
+        for name, (genc, genh, gent) in self._module.items():
             if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
                 continue
             genc.write(output_dir)
             genh.write(output_dir)
+            gent.write(output_dir)
 
     def _begin_builtin_module(self) -> None:
         pass
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..3adf0319cf 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_points: bool = False) -> None:
     """
     Generate C code for the given schema into the target directory.
 
@@ -49,7 +50,7 @@ def generate(schema_file: str,
     schema = QAPISchema(schema_file)
     gen_types(schema, output_dir, prefix, builtins)
     gen_visit(schema, output_dir, prefix, builtins)
-    gen_commands(schema, output_dir, prefix)
+    gen_commands(schema, output_dir, prefix, add_trace_points)
     gen_events(schema, output_dir, prefix)
     gen_introspect(schema, output_dir, prefix, unmask)
 
@@ -74,6 +75,8 @@ def main() -> int:
     parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
                         dest='unmask',
                         help="expose non-ABI names in introspection")
+    parser.add_argument('--add-trace-points', action='store_true',
+                        help="add trace points 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_points=args.add_trace_points)
     except QAPIError as err:
         print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
         return 1
-- 
2.31.1

[PATCH v2 4/4] meson: generate trace points for qmp commands

[Vladimir Sementsov-Ogievskiy]

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

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

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

Re: [PATCH v2 1/4] jobs: drop qmp_ trace points

[Stefan Hajnoczi]

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

On Thu, Dec 23, 2021 at 12:07:53PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/block/trace-events b/block/trace-events
> index 549090d453..5be3e3913b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -49,15 +49,6 @@ block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64"
>  block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
>  block_copy_write_zeroes_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
>  
> -# ../blockdev.c
> -qmp_block_job_cancel(void *job) "job %p"
> -qmp_block_job_pause(void *job) "job %p"
> -qmp_block_job_resume(void *job) "job %p"
> -qmp_block_job_complete(void *job) "job %p"
> -qmp_block_job_finalize(void *job) "job %p"
> -qmp_block_job_dismiss(void *job) "job %p"
> -qmp_block_stream(void *bs) "bs %p"
> -
>  # file-win32.c
>  file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d"
>  
> diff --git a/trace-events b/trace-events
> index a637a61eba..1265f1e0cc 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -79,14 +79,6 @@ job_state_transition(void *job,  int ret, const char *legal, const char *s0, con
>  job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
>  job_completed(void *job, int ret) "job %p ret %d"
>  
> -# job-qmp.c
> -qmp_job_cancel(void *job) "job %p"
> -qmp_job_pause(void *job) "job %p"
> -qmp_job_resume(void *job) "job %p"
> -qmp_job_complete(void *job) "job %p"
> -qmp_job_finalize(void *job) "job %p"
> -qmp_job_dismiss(void *job) "job %p"

The job pointer argument will be lost. That's not ideal but probably
worth getting trace events for all QMP commands.

Stefan

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

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Stefan Hajnoczi]

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

On Thu, Dec 23, 2021 at 12:07:54PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add possibility to generate trace points for each qmp command.
> 
> We should generate both trace points and trace-events file, for further
> trace point code generation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21001bbd6b..9691c11f96 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_points: bool) -> str:

Please use the term "trace events" instead of "trace points". That's the
term that docs/devel/tracing.rst uses.

> @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
>                   proto=build_marshal_proto(name))
>  
>  
> +def gen_trace(name: str) -> str:
> +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'

This trace event is emitted in 3 different ways:
1. For arguments before calling a QMP command.
2. For the error message when the QMP command fails.
3. For the return value when a QMP command succeeds.

This makes parsing the trace akward because you get two events in
succession for a single call and they both have the same name.

Please generate 2 trace events:
1. qmp_enter_<name> <args>
2. qmp_exit_<name> <ret> <succeeded>

(That's similar to how the syscalls Linux kernel trace events work.)

Scripts processing the trace can easily differentiate between enter
(args) and exit (return value) events without parsing or keeping state
to count the second event.

Thanks,
Stefan

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

Re: [PATCH v2 3/4] scripts/qapi-gen.py: add --add-trace-points option

[Stefan Hajnoczi]

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

On Thu, Dec 23, 2021 at 12:07:55PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> @@ -74,6 +75,8 @@ def main() -> int:
>      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>                          dest='unmask',
>                          help="expose non-ABI names in introspection")
> +    parser.add_argument('--add-trace-points', action='store_true',
> +                        help="add trace points to qmp marshals")

Please call it --add-trace-events for consistency with QEMU tracing
terminology.

Thanks,
Stefan

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

Re: [PATCH v2 4/4] meson: generate trace points for qmp commands

[Stefan Hajnoczi]

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

On Thu, Dec 23, 2021 at 12:07:56PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> 1. Use --add-trace-points when generate qmp commands
> 2. Add corresponding .trace-events files as outputs in qapi_files
>    custom target
> 3. Define global qapi_trace_events list of .trace-events file targets,
>    to fill in trace/qapi.build and to use in trace/meson.build
> 4. In trace/meson.build use the new array as an additional source of
>    .trace_events files to be processed
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  meson.build       |  1 +
>  qapi/meson.build  |  9 ++++++++-
>  trace/meson.build | 11 ++++++++---
>  3 files changed, 17 insertions(+), 4 deletions(-)

This patch has a meson subdir ordering dependency: qapi/ must be
processed before trace/. Please document this in meson.build so anyone
editing subdirs knows to preserve this order.

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

Re: [PATCH v2 1/4] jobs: drop qmp_ trace points

[John Snow]

On Mon, Jan 10, 2022 at 11:06 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Dec 23, 2021 at 12:07:53PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> > diff --git a/block/trace-events b/block/trace-events
> > index 549090d453..5be3e3913b 100644
> > --- a/block/trace-events
> > +++ b/block/trace-events
> > @@ -49,15 +49,6 @@ block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64"
> >  block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
> >  block_copy_write_zeroes_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
> >
> > -# ../blockdev.c
> > -qmp_block_job_cancel(void *job) "job %p"
> > -qmp_block_job_pause(void *job) "job %p"
> > -qmp_block_job_resume(void *job) "job %p"
> > -qmp_block_job_complete(void *job) "job %p"
> > -qmp_block_job_finalize(void *job) "job %p"
> > -qmp_block_job_dismiss(void *job) "job %p"
> > -qmp_block_stream(void *bs) "bs %p"
> > -
> >  # file-win32.c
> >  file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d"
> >
> > diff --git a/trace-events b/trace-events
> > index a637a61eba..1265f1e0cc 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -79,14 +79,6 @@ job_state_transition(void *job,  int ret, const char *legal, const char *s0, con
> >  job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
> >  job_completed(void *job, int ret) "job %p ret %d"
> >
> > -# job-qmp.c
> > -qmp_job_cancel(void *job) "job %p"
> > -qmp_job_pause(void *job) "job %p"
> > -qmp_job_resume(void *job) "job %p"
> > -qmp_job_complete(void *job) "job %p"
> > -qmp_job_finalize(void *job) "job %p"
> > -qmp_job_dismiss(void *job) "job %p"
>
> The job pointer argument will be lost. That's not ideal but probably
> worth getting trace events for all QMP commands.
>
> Stefan

We could move the six job-related tracepoints into the implementation
routines instead; i.e. job_user_cancel, job_user_pause, etc. This
would cover all 12 QMP interface tracepoints, and that'd let us keep
the "implementation" trace points.

--js

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[John Snow]

On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> Add possibility to generate trace points for each qmp command.
>
> We should generate both trace points and trace-events file, for further
> trace point code generation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21001bbd6b..9691c11f96 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_points: bool) -> str:
>      ret = ''
>
>      argstr = ''
> @@ -71,21 +72,65 @@ def gen_call(name: str,
>      if ret_type:
>          lhs = 'retval = '
>
> -    ret = mcgen('''
> +    qmp_name = f'qmp_{c_name(name)}'
> +    upper = qmp_name.upper()
> +
> +    if add_trace_points:
> +        ret += mcgen('''
> +
> +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> +        trace_%(qmp_name)s("", req_json->str);
> +    }
> +    ''',
> +                     upper=upper, qmp_name=qmp_name)
> +
> +    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) {
> +''')
> +
> +    if add_trace_points:
> +        ret += mcgen('''
> +        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
> +''',
> +                     qmp_name=qmp_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_points:
> +        if ret_type:
> +            ret += mcgen('''
> +
> +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
> +        trace_%(qmp_name)s("RET:", ret_json->str);
> +    }
> +    ''',
> +                     upper=upper, qmp_name=qmp_name)
> +        else:
> +            ret += mcgen('''
> +
> +    trace_%(qmp_name)s("SUCCESS", "");
> +    ''',
> +                         qmp_name=qmp_name)
> +
>      return ret
>
>
> @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
>                   proto=build_marshal_proto(name))
>
>
> +def gen_trace(name: str) -> str:
> +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
> +
>  def gen_marshal(name: str,
>                  arg_type: Optional[QAPISchemaObjectType],
>                  boxed: bool,
> -                ret_type: Optional[QAPISchemaType]) -> str:
> +                ret_type: Optional[QAPISchemaType],
> +                add_trace_points: 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 +229,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_points)
>
>      ret += mcgen('''
>
> @@ -238,11 +287,12 @@ def gen_register_command(name: str,
>
>
>  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> -    def __init__(self, prefix: str):
> +    def __init__(self, prefix: str, add_trace_points: bool):
>          super().__init__(
>              prefix, 'qapi-commands',
>              ' * Schema-defined QAPI/QMP commands', None, __doc__)
>          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> +        self.add_trace_points = add_trace_points
>
>      def _begin_user_module(self, name: str) -> None:
>          self._visited_ret_types[self._genc] = set()
> @@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
>
>  ''',
>                               commands=commands, visit=visit))
> +
> +        if self.add_trace_points and c_name(commands) != 'qapi_commands':
> +            self._genc.add(mcgen('''
> +#include "trace/trace-qapi.h"
> +#include "qapi/qmp/qjson.h"
> +#include "trace/trace-%(nm)s_trace_events.h"
> +''',
> +                                 nm=c_name(commands)))
> +
>          self._genh.add(mcgen('''
>  #include "%(types)s.h"
>
> @@ -322,7 +381,9 @@ def visit_command(self,
>          with ifcontext(ifcond, self._genh, self._genc):
>              self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>              self._genh.add(gen_marshal_decl(name))
> -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> +                                       self.add_trace_points))
> +            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 +393,8 @@ def visit_command(self,
>
>  def gen_commands(schema: QAPISchema,
>                   output_dir: str,
> -                 prefix: str) -> None:
> -    vis = QAPISchemaGenCommandVisitor(prefix)
> +                 prefix: str,
> +                 add_trace_points: bool) -> None:
> +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
>      schema.visit(vis)
>      vis.write(output_dir)
> --
> 2.31.1
>

I looked at Stefan's feedback and I want to second his recommendation
to use _enter and _exit tracepoints, this closely resembles a lot of
temporary debugging code I've written for jobs/bitmaps over the years
and find the technique helpful.

--js

(as a tangent ...

I wish I had a much nicer way of doing C generation from Python, this
is so ugly. It's not your fault, of course. I'm just wondering if the
mailing list has any smarter ideas for future improvements to the
mcgen interface, because I find this type of code really hard to
review.)

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[John Snow]

On Tue, Jan 11, 2022 at 6:53 PM John Snow <jsnow@redhat.com> wrote:
>
> On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
> >
> > Add possibility to generate trace points for each qmp command.
> >
> > We should generate both trace points and trace-events file, for further
> > trace point code generation.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 73 insertions(+), 11 deletions(-)
> >
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 21001bbd6b..9691c11f96 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_points: bool) -> str:
> >      ret = ''
> >
> >      argstr = ''
> > @@ -71,21 +72,65 @@ def gen_call(name: str,
> >      if ret_type:
> >          lhs = 'retval = '
> >
> > -    ret = mcgen('''
> > +    qmp_name = f'qmp_{c_name(name)}'
> > +    upper = qmp_name.upper()
> > +
> > +    if add_trace_points:
> > +        ret += mcgen('''
> > +
> > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> > +        trace_%(qmp_name)s("", req_json->str);
> > +    }
> > +    ''',
> > +                     upper=upper, qmp_name=qmp_name)
> > +
> > +    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) {
> > +''')
> > +
> > +    if add_trace_points:
> > +        ret += mcgen('''
> > +        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
> > +''',
> > +                     qmp_name=qmp_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_points:
> > +        if ret_type:
> > +            ret += mcgen('''
> > +
> > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
> > +        trace_%(qmp_name)s("RET:", ret_json->str);
> > +    }
> > +    ''',
> > +                     upper=upper, qmp_name=qmp_name)
> > +        else:
> > +            ret += mcgen('''
> > +
> > +    trace_%(qmp_name)s("SUCCESS", "");
> > +    ''',
> > +                         qmp_name=qmp_name)
> > +
> >      return ret
> >
> >
> > @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
> >                   proto=build_marshal_proto(name))
> >
> >
> > +def gen_trace(name: str) -> str:
> > +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
> > +
> >  def gen_marshal(name: str,
> >                  arg_type: Optional[QAPISchemaObjectType],
> >                  boxed: bool,
> > -                ret_type: Optional[QAPISchemaType]) -> str:
> > +                ret_type: Optional[QAPISchemaType],
> > +                add_trace_points: 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 +229,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_points)
> >
> >      ret += mcgen('''
> >
> > @@ -238,11 +287,12 @@ def gen_register_command(name: str,
> >
> >
> >  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> > -    def __init__(self, prefix: str):
> > +    def __init__(self, prefix: str, add_trace_points: bool):
> >          super().__init__(
> >              prefix, 'qapi-commands',
> >              ' * Schema-defined QAPI/QMP commands', None, __doc__)
> >          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> > +        self.add_trace_points = add_trace_points
> >
> >      def _begin_user_module(self, name: str) -> None:
> >          self._visited_ret_types[self._genc] = set()
> > @@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
> >
> >  ''',
> >                               commands=commands, visit=visit))
> > +
> > +        if self.add_trace_points and c_name(commands) != 'qapi_commands':
> > +            self._genc.add(mcgen('''
> > +#include "trace/trace-qapi.h"
> > +#include "qapi/qmp/qjson.h"
> > +#include "trace/trace-%(nm)s_trace_events.h"
> > +''',
> > +                                 nm=c_name(commands)))
> > +
> >          self._genh.add(mcgen('''
> >  #include "%(types)s.h"
> >
> > @@ -322,7 +381,9 @@ def visit_command(self,
> >          with ifcontext(ifcond, self._genh, self._genc):
> >              self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> >              self._genh.add(gen_marshal_decl(name))
> > -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> > +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> > +                                       self.add_trace_points))
> > +            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 +393,8 @@ def visit_command(self,
> >
> >  def gen_commands(schema: QAPISchema,
> >                   output_dir: str,
> > -                 prefix: str) -> None:
> > -    vis = QAPISchemaGenCommandVisitor(prefix)
> > +                 prefix: str,
> > +                 add_trace_points: bool) -> None:
> > +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
> >      schema.visit(vis)
> >      vis.write(output_dir)
> > --
> > 2.31.1
> >
>
> I looked at Stefan's feedback and I want to second his recommendation
> to use _enter and _exit tracepoints, this closely resembles a lot of
> temporary debugging code I've written for jobs/bitmaps over the years
> and find the technique helpful.
>
> --js
>
> (as a tangent ...
>
> I wish I had a much nicer way of doing C generation from Python, this
> is so ugly. It's not your fault, of course. I'm just wondering if the
> mailing list has any smarter ideas for future improvements to the
> mcgen interface, because I find this type of code really hard to
> review.)

Come to think of it, we could use a framework that was originally
designed for HTML templating, but for C instead. Wait! Don't close the
email yet, I have some funny things to write still!!

Downsides:
- New template language
- Complex

Pros:
- Easier to read
- Easier to review
- Avoids reinventing the wheel
- Doesn't even technically add a dependency, since Sphinx already
relies on Jinja ...
- *Extremely* funny

As an example, let's say we had a file
"scripts/qapi/templates/qmp_marshal_output.c" that looked like this:
```
static void qmp_marshal_output_{{c_name}}(
    {{c_type}} ret_in,
    QObject **ret_out,
    Error **errp
) {
    Visitor *v;

    v = qobject_output_visitor_new_qmp(ret_out);
    if (visit_type_{{c_name}}(v, "unused", &ret_in, errp)) {
        visit_complete(v, ret_out);
    }
    visit_free(v);
    v = qapi_dealloc_visitor_new();
    visit_type_{{c_name}}(v, "unused", &ret_in, NULL);
    visit_free(v);
}
```

We could use Jinja to process it from Python like this:

```
import os
from jinja2 import PackageLoader, Environment, FileSystemLoader

env = Environment(loader = FileSystemLoader('./templates/'))
template = env.get_template("qmp_marshal_output.c")
rendered = cgen(template.render(
    c_name = "foo",
    c_type = "int",
))

print(rendered)
```

You'd get output like this:

```
static void qmp_marshal_output_foo(
    int ret_in,
    QObject **ret_out,
    Error **errp
) {
    Visitor *v;

    v = qobject_output_visitor_new_qmp(ret_out);
    if (visit_type_foo(v, "unused", &ret_in, errp)) {
        visit_complete(v, ret_out);
    }
    visit_free(v);
    v = qapi_dealloc_visitor_new();
    visit_type_foo(v, "unused", &ret_in, NULL);
    visit_free(v);
```

Jinja also supports conditionals and some other logic constructs, so
it'd *probably* apply to most templates we'd want to write, though
admittedly I haven't thought through if it'd actually work everywhere
we'd need it, and I am mostly having a laugh.

...OK, back to work!

--js

Re: [PATCH v2 3/4] scripts/qapi-gen.py: add --add-trace-points option

[John Snow]

On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> Add and option to generate trace points. We should generate both trace
> points and trace-events files for further trace point code generation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  scripts/qapi/gen.py  | 13 ++++++++++---
>  scripts/qapi/main.py | 10 +++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..605b3fe68a 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -251,7 +251,7 @@ def __init__(self,
>          self._builtin_blurb = builtin_blurb
>          self._pydoc = pydoc
>          self._current_module: Optional[str] = None
> -        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
>          self._main_module: Optional[str] = None
>
>      @property
> @@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
>          assert self._current_module is not None
>          return self._module[self._current_module][1]
>
> +    @property
> +    def _gent(self) -> QAPIGen:
> +        assert self._current_module is not None
> +        return self._module[self._current_module][2]
> +
>      @staticmethod
>      def _module_dirname(name: str) -> str:
>          if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
>          basename = self._module_filename(self._what, name)
>          genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>          genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> -        self._module[name] = (genc, genh)
> +        gent = QAPIGen(basename + '.trace-events')
> +        self._module[name] = (genc, genh, gent)
>          self._current_module = name
>
>      @contextmanager
> @@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
>          self._current_module = old_module
>
>      def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> -        for name, (genc, genh) in self._module.items():
> +        for name, (genc, genh, gent) in self._module.items():
>              if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>                  continue
>              genc.write(output_dir)
>              genh.write(output_dir)
> +            gent.write(output_dir)
>
>      def _begin_builtin_module(self) -> None:
>          pass
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..3adf0319cf 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_points: bool = False) -> None:
>      """
>      Generate C code for the given schema into the target directory.
>
> @@ -49,7 +50,7 @@ def generate(schema_file: str,
>      schema = QAPISchema(schema_file)
>      gen_types(schema, output_dir, prefix, builtins)
>      gen_visit(schema, output_dir, prefix, builtins)
> -    gen_commands(schema, output_dir, prefix)
> +    gen_commands(schema, output_dir, prefix, add_trace_points)
>      gen_events(schema, output_dir, prefix)
>      gen_introspect(schema, output_dir, prefix, unmask)
>
> @@ -74,6 +75,8 @@ def main() -> int:
>      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>                          dest='unmask',
>                          help="expose non-ABI names in introspection")
> +    parser.add_argument('--add-trace-points', action='store_true',
> +                        help="add trace points to qmp marshals")

"Add trace events to generated marshaling functions." maybe?

>      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_points=args.add_trace_points)
>      except QAPIError as err:
>          print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
>          return 1
> --
> 2.31.1
>

I suppose the flag is so that non-QEMU invocations of the QAPI
generator (for tests, etc) will compile correctly without tracepoint
definitions, yeah?

Re: [PATCH v2 1/4] jobs: drop qmp_ trace points

[Stefan Hajnoczi]

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

On Tue, Jan 11, 2022 at 06:44:58PM -0500, John Snow wrote:
> On Mon, Jan 10, 2022 at 11:06 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Thu, Dec 23, 2021 at 12:07:53PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> > > diff --git a/block/trace-events b/block/trace-events
> > > index 549090d453..5be3e3913b 100644
> > > --- a/block/trace-events
> > > +++ b/block/trace-events
> > > @@ -49,15 +49,6 @@ block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64"
> > >  block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
> > >  block_copy_write_zeroes_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
> > >
> > > -# ../blockdev.c
> > > -qmp_block_job_cancel(void *job) "job %p"
> > > -qmp_block_job_pause(void *job) "job %p"
> > > -qmp_block_job_resume(void *job) "job %p"
> > > -qmp_block_job_complete(void *job) "job %p"
> > > -qmp_block_job_finalize(void *job) "job %p"
> > > -qmp_block_job_dismiss(void *job) "job %p"
> > > -qmp_block_stream(void *bs) "bs %p"
> > > -
> > >  # file-win32.c
> > >  file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "acb %p opaque %p offset %"PRId64" count %d type %d"
> > >
> > > diff --git a/trace-events b/trace-events
> > > index a637a61eba..1265f1e0cc 100644
> > > --- a/trace-events
> > > +++ b/trace-events
> > > @@ -79,14 +79,6 @@ job_state_transition(void *job,  int ret, const char *legal, const char *s0, con
> > >  job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
> > >  job_completed(void *job, int ret) "job %p ret %d"
> > >
> > > -# job-qmp.c
> > > -qmp_job_cancel(void *job) "job %p"
> > > -qmp_job_pause(void *job) "job %p"
> > > -qmp_job_resume(void *job) "job %p"
> > > -qmp_job_complete(void *job) "job %p"
> > > -qmp_job_finalize(void *job) "job %p"
> > > -qmp_job_dismiss(void *job) "job %p"
> >
> > The job pointer argument will be lost. That's not ideal but probably
> > worth getting trace events for all QMP commands.
> >
> > Stefan
> 
> We could move the six job-related tracepoints into the implementation
> routines instead; i.e. job_user_cancel, job_user_pause, etc. This
> would cover all 12 QMP interface tracepoints, and that'd let us keep
> the "implementation" trace points.

Good idea. Having the job pointer might be handy so it's worth
preserving these trace events.

Stefan

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

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Stefan Hajnoczi]

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

On Tue, Jan 11, 2022 at 07:32:52PM -0500, John Snow wrote:
> On Tue, Jan 11, 2022 at 6:53 PM John Snow <jsnow@redhat.com> wrote:
> >
> > On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> > >
> > > Add possibility to generate trace points for each qmp command.
> > >
> > > We should generate both trace points and trace-events file, for further
> > > trace point code generation.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > >  scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 73 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > > index 21001bbd6b..9691c11f96 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_points: bool) -> str:
> > >      ret = ''
> > >
> > >      argstr = ''
> > > @@ -71,21 +72,65 @@ def gen_call(name: str,
> > >      if ret_type:
> > >          lhs = 'retval = '
> > >
> > > -    ret = mcgen('''
> > > +    qmp_name = f'qmp_{c_name(name)}'
> > > +    upper = qmp_name.upper()
> > > +
> > > +    if add_trace_points:
> > > +        ret += mcgen('''
> > > +
> > > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > > +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> > > +        trace_%(qmp_name)s("", req_json->str);
> > > +    }
> > > +    ''',
> > > +                     upper=upper, qmp_name=qmp_name)
> > > +
> > > +    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) {
> > > +''')
> > > +
> > > +    if add_trace_points:
> > > +        ret += mcgen('''
> > > +        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
> > > +''',
> > > +                     qmp_name=qmp_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_points:
> > > +        if ret_type:
> > > +            ret += mcgen('''
> > > +
> > > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > > +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
> > > +        trace_%(qmp_name)s("RET:", ret_json->str);
> > > +    }
> > > +    ''',
> > > +                     upper=upper, qmp_name=qmp_name)
> > > +        else:
> > > +            ret += mcgen('''
> > > +
> > > +    trace_%(qmp_name)s("SUCCESS", "");
> > > +    ''',
> > > +                         qmp_name=qmp_name)
> > > +
> > >      return ret
> > >
> > >
> > > @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
> > >                   proto=build_marshal_proto(name))
> > >
> > >
> > > +def gen_trace(name: str) -> str:
> > > +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
> > > +
> > >  def gen_marshal(name: str,
> > >                  arg_type: Optional[QAPISchemaObjectType],
> > >                  boxed: bool,
> > > -                ret_type: Optional[QAPISchemaType]) -> str:
> > > +                ret_type: Optional[QAPISchemaType],
> > > +                add_trace_points: 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 +229,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_points)
> > >
> > >      ret += mcgen('''
> > >
> > > @@ -238,11 +287,12 @@ def gen_register_command(name: str,
> > >
> > >
> > >  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> > > -    def __init__(self, prefix: str):
> > > +    def __init__(self, prefix: str, add_trace_points: bool):
> > >          super().__init__(
> > >              prefix, 'qapi-commands',
> > >              ' * Schema-defined QAPI/QMP commands', None, __doc__)
> > >          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> > > +        self.add_trace_points = add_trace_points
> > >
> > >      def _begin_user_module(self, name: str) -> None:
> > >          self._visited_ret_types[self._genc] = set()
> > > @@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
> > >
> > >  ''',
> > >                               commands=commands, visit=visit))
> > > +
> > > +        if self.add_trace_points and c_name(commands) != 'qapi_commands':
> > > +            self._genc.add(mcgen('''
> > > +#include "trace/trace-qapi.h"
> > > +#include "qapi/qmp/qjson.h"
> > > +#include "trace/trace-%(nm)s_trace_events.h"
> > > +''',
> > > +                                 nm=c_name(commands)))
> > > +
> > >          self._genh.add(mcgen('''
> > >  #include "%(types)s.h"
> > >
> > > @@ -322,7 +381,9 @@ def visit_command(self,
> > >          with ifcontext(ifcond, self._genh, self._genc):
> > >              self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> > >              self._genh.add(gen_marshal_decl(name))
> > > -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> > > +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> > > +                                       self.add_trace_points))
> > > +            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 +393,8 @@ def visit_command(self,
> > >
> > >  def gen_commands(schema: QAPISchema,
> > >                   output_dir: str,
> > > -                 prefix: str) -> None:
> > > -    vis = QAPISchemaGenCommandVisitor(prefix)
> > > +                 prefix: str,
> > > +                 add_trace_points: bool) -> None:
> > > +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
> > >      schema.visit(vis)
> > >      vis.write(output_dir)
> > > --
> > > 2.31.1
> > >
> >
> > I looked at Stefan's feedback and I want to second his recommendation
> > to use _enter and _exit tracepoints, this closely resembles a lot of
> > temporary debugging code I've written for jobs/bitmaps over the years
> > and find the technique helpful.
> >
> > --js
> >
> > (as a tangent ...
> >
> > I wish I had a much nicer way of doing C generation from Python, this
> > is so ugly. It's not your fault, of course. I'm just wondering if the
> > mailing list has any smarter ideas for future improvements to the
> > mcgen interface, because I find this type of code really hard to
> > review.)
> 
> Come to think of it, we could use a framework that was originally
> designed for HTML templating, but for C instead. Wait! Don't close the
> email yet, I have some funny things to write still!!
> 
> Downsides:
> - New template language
> - Complex
> 
> Pros:
> - Easier to read
> - Easier to review
> - Avoids reinventing the wheel
> - Doesn't even technically add a dependency, since Sphinx already
> relies on Jinja ...
> - *Extremely* funny
> 
> As an example, let's say we had a file
> "scripts/qapi/templates/qmp_marshal_output.c" that looked like this:
> ```
> static void qmp_marshal_output_{{c_name}}(
>     {{c_type}} ret_in,
>     QObject **ret_out,
>     Error **errp
> ) {
>     Visitor *v;
> 
>     v = qobject_output_visitor_new_qmp(ret_out);
>     if (visit_type_{{c_name}}(v, "unused", &ret_in, errp)) {
>         visit_complete(v, ret_out);
>     }
>     visit_free(v);
>     v = qapi_dealloc_visitor_new();
>     visit_type_{{c_name}}(v, "unused", &ret_in, NULL);
>     visit_free(v);
> }
> ```
> 
> We could use Jinja to process it from Python like this:
> 
> ```
> import os
> from jinja2 import PackageLoader, Environment, FileSystemLoader
> 
> env = Environment(loader = FileSystemLoader('./templates/'))
> template = env.get_template("qmp_marshal_output.c")
> rendered = cgen(template.render(
>     c_name = "foo",
>     c_type = "int",
> ))
> 
> print(rendered)
> ```
> 
> You'd get output like this:
> 
> ```
> static void qmp_marshal_output_foo(
>     int ret_in,
>     QObject **ret_out,
>     Error **errp
> ) {
>     Visitor *v;
> 
>     v = qobject_output_visitor_new_qmp(ret_out);
>     if (visit_type_foo(v, "unused", &ret_in, errp)) {
>         visit_complete(v, ret_out);
>     }
>     visit_free(v);
>     v = qapi_dealloc_visitor_new();
>     visit_type_foo(v, "unused", &ret_in, NULL);
>     visit_free(v);
> ```
> 
> Jinja also supports conditionals and some other logic constructs, so
> it'd *probably* apply to most templates we'd want to write, though
> admittedly I haven't thought through if it'd actually work everywhere
> we'd need it, and I am mostly having a laugh.
> 
> ...OK, back to work!

I agree that mcgen string formatting is very hard to review.

I'm not sure if it's possible to separate the C templates into large
chunks that are easy to read though. If there are many small C templates
then it's hard to review and I think that's the core problem, not the
string formatting/expansion mechanism (%(var)s vs Python {var} vs jinja2
{{var}}).

If we could stick to Python standard library features instead of jinja2
that would be nice because while jinja2 is powerful and widely-used,
it's probably a bit too powerful and adds complexity/boilerplate that
could be overkill in this situation.

Stefan

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

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Vladimir Sementsov-Ogievskiy]

12.01.2022 13:50, Stefan Hajnoczi wrote:
> On Tue, Jan 11, 2022 at 07:32:52PM -0500, John Snow wrote:
>> On Tue, Jan 11, 2022 at 6:53 PM John Snow <jsnow@redhat.com> wrote:
>>>
>>> On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> wrote:
>>>>
>>>> Add possibility to generate trace points for each qmp command.
>>>>
>>>> We should generate both trace points and trace-events file, for further
>>>> trace point code generation.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
>>>>   1 file changed, 73 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>>> index 21001bbd6b..9691c11f96 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_points: bool) -> str:
>>>>       ret = ''
>>>>
>>>>       argstr = ''
>>>> @@ -71,21 +72,65 @@ def gen_call(name: str,
>>>>       if ret_type:
>>>>           lhs = 'retval = '
>>>>
>>>> -    ret = mcgen('''
>>>> +    qmp_name = f'qmp_{c_name(name)}'
>>>> +    upper = qmp_name.upper()
>>>> +
>>>> +    if add_trace_points:
>>>> +        ret += mcgen('''
>>>> +
>>>> +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
>>>> +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
>>>> +        trace_%(qmp_name)s("", req_json->str);
>>>> +    }
>>>> +    ''',
>>>> +                     upper=upper, qmp_name=qmp_name)
>>>> +
>>>> +    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) {
>>>> +''')
>>>> +
>>>> +    if add_trace_points:
>>>> +        ret += mcgen('''
>>>> +        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
>>>> +''',
>>>> +                     qmp_name=qmp_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_points:
>>>> +        if ret_type:
>>>> +            ret += mcgen('''
>>>> +
>>>> +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
>>>> +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
>>>> +        trace_%(qmp_name)s("RET:", ret_json->str);
>>>> +    }
>>>> +    ''',
>>>> +                     upper=upper, qmp_name=qmp_name)
>>>> +        else:
>>>> +            ret += mcgen('''
>>>> +
>>>> +    trace_%(qmp_name)s("SUCCESS", "");
>>>> +    ''',
>>>> +                         qmp_name=qmp_name)
>>>> +
>>>>       return ret
>>>>
>>>>
>>>> @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
>>>>                    proto=build_marshal_proto(name))
>>>>
>>>>
>>>> +def gen_trace(name: str) -> str:
>>>> +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
>>>> +
>>>>   def gen_marshal(name: str,
>>>>                   arg_type: Optional[QAPISchemaObjectType],
>>>>                   boxed: bool,
>>>> -                ret_type: Optional[QAPISchemaType]) -> str:
>>>> +                ret_type: Optional[QAPISchemaType],
>>>> +                add_trace_points: 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 +229,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_points)
>>>>
>>>>       ret += mcgen('''
>>>>
>>>> @@ -238,11 +287,12 @@ def gen_register_command(name: str,
>>>>
>>>>
>>>>   class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>>>> -    def __init__(self, prefix: str):
>>>> +    def __init__(self, prefix: str, add_trace_points: bool):
>>>>           super().__init__(
>>>>               prefix, 'qapi-commands',
>>>>               ' * Schema-defined QAPI/QMP commands', None, __doc__)
>>>>           self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
>>>> +        self.add_trace_points = add_trace_points
>>>>
>>>>       def _begin_user_module(self, name: str) -> None:
>>>>           self._visited_ret_types[self._genc] = set()
>>>> @@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
>>>>
>>>>   ''',
>>>>                                commands=commands, visit=visit))
>>>> +
>>>> +        if self.add_trace_points and c_name(commands) != 'qapi_commands':
>>>> +            self._genc.add(mcgen('''
>>>> +#include "trace/trace-qapi.h"
>>>> +#include "qapi/qmp/qjson.h"
>>>> +#include "trace/trace-%(nm)s_trace_events.h"
>>>> +''',
>>>> +                                 nm=c_name(commands)))
>>>> +
>>>>           self._genh.add(mcgen('''
>>>>   #include "%(types)s.h"
>>>>
>>>> @@ -322,7 +381,9 @@ def visit_command(self,
>>>>           with ifcontext(ifcond, self._genh, self._genc):
>>>>               self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>>>>               self._genh.add(gen_marshal_decl(name))
>>>> -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>>>> +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
>>>> +                                       self.add_trace_points))
>>>> +            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 +393,8 @@ def visit_command(self,
>>>>
>>>>   def gen_commands(schema: QAPISchema,
>>>>                    output_dir: str,
>>>> -                 prefix: str) -> None:
>>>> -    vis = QAPISchemaGenCommandVisitor(prefix)
>>>> +                 prefix: str,
>>>> +                 add_trace_points: bool) -> None:
>>>> +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
>>>>       schema.visit(vis)
>>>>       vis.write(output_dir)
>>>> --
>>>> 2.31.1
>>>>
>>>
>>> I looked at Stefan's feedback and I want to second his recommendation
>>> to use _enter and _exit tracepoints, this closely resembles a lot of
>>> temporary debugging code I've written for jobs/bitmaps over the years
>>> and find the technique helpful.
>>>
>>> --js
>>>
>>> (as a tangent ...
>>>
>>> I wish I had a much nicer way of doing C generation from Python, this
>>> is so ugly. It's not your fault, of course. I'm just wondering if the
>>> mailing list has any smarter ideas for future improvements to the
>>> mcgen interface, because I find this type of code really hard to
>>> review.)
>>
>> Come to think of it, we could use a framework that was originally
>> designed for HTML templating, but for C instead. Wait! Don't close the
>> email yet, I have some funny things to write still!!
>>
>> Downsides:
>> - New template language
>> - Complex
>>
>> Pros:
>> - Easier to read
>> - Easier to review
>> - Avoids reinventing the wheel
>> - Doesn't even technically add a dependency, since Sphinx already
>> relies on Jinja ...
>> - *Extremely* funny
>>
>> As an example, let's say we had a file
>> "scripts/qapi/templates/qmp_marshal_output.c" that looked like this:
>> ```
>> static void qmp_marshal_output_{{c_name}}(
>>      {{c_type}} ret_in,
>>      QObject **ret_out,
>>      Error **errp
>> ) {
>>      Visitor *v;
>>
>>      v = qobject_output_visitor_new_qmp(ret_out);
>>      if (visit_type_{{c_name}}(v, "unused", &ret_in, errp)) {
>>          visit_complete(v, ret_out);
>>      }
>>      visit_free(v);
>>      v = qapi_dealloc_visitor_new();
>>      visit_type_{{c_name}}(v, "unused", &ret_in, NULL);
>>      visit_free(v);
>> }
>> ```
>>
>> We could use Jinja to process it from Python like this:
>>
>> ```
>> import os
>> from jinja2 import PackageLoader, Environment, FileSystemLoader
>>
>> env = Environment(loader = FileSystemLoader('./templates/'))
>> template = env.get_template("qmp_marshal_output.c")
>> rendered = cgen(template.render(
>>      c_name = "foo",
>>      c_type = "int",
>> ))
>>
>> print(rendered)
>> ```
>>
>> You'd get output like this:
>>
>> ```
>> static void qmp_marshal_output_foo(
>>      int ret_in,
>>      QObject **ret_out,
>>      Error **errp
>> ) {
>>      Visitor *v;
>>
>>      v = qobject_output_visitor_new_qmp(ret_out);
>>      if (visit_type_foo(v, "unused", &ret_in, errp)) {
>>          visit_complete(v, ret_out);
>>      }
>>      visit_free(v);
>>      v = qapi_dealloc_visitor_new();
>>      visit_type_foo(v, "unused", &ret_in, NULL);
>>      visit_free(v);
>> ```
>>
>> Jinja also supports conditionals and some other logic constructs, so
>> it'd *probably* apply to most templates we'd want to write, though
>> admittedly I haven't thought through if it'd actually work everywhere
>> we'd need it, and I am mostly having a laugh.
>>
>> ...OK, back to work!
> 
> I agree that mcgen string formatting is very hard to review.
> 
> I'm not sure if it's possible to separate the C templates into large
> chunks that are easy to read though. If there are many small C templates
> then it's hard to review and I think that's the core problem, not the
> string formatting/expansion mechanism (%(var)s vs Python {var} vs jinja2
> {{var}}).
> 
> If we could stick to Python standard library features instead of jinja2
> that would be nice because while jinja2 is powerful and widely-used,
> it's probably a bit too powerful and adds complexity/boilerplate that
> could be overkill in this situation.
> 
> Stefan
> 

I think that conversion to Python f-strings would be a good start: not very invasive, no extra dependencies, but may probably solve 60-80 % of the problem.

I thought about different templating techniques when creating scripts/block-coroutine-wrapper.py, but finally it just utilizes f-string (look at line 119 for example).


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Vladimir Sementsov-Ogievskiy]

10.01.2022 19:22, Stefan Hajnoczi wrote:
> On Thu, Dec 23, 2021 at 12:07:54PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Add possibility to generate trace points for each qmp command.
>>
>> We should generate both trace points and trace-events file, for further
>> trace point code generation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
>>   1 file changed, 73 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 21001bbd6b..9691c11f96 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_points: bool) -> str:
> 
> Please use the term "trace events" instead of "trace points". That's the
> term that docs/devel/tracing.rst uses.
> 
>> @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
>>                    proto=build_marshal_proto(name))
>>   
>>   
>> +def gen_trace(name: str) -> str:
>> +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
> 
> This trace event is emitted in 3 different ways:
> 1. For arguments before calling a QMP command.
> 2. For the error message when the QMP command fails.
> 3. For the return value when a QMP command succeeds.
> 
> This makes parsing the trace akward because you get two events in
> succession for a single call and they both have the same name.
> 
> Please generate 2 trace events:
> 1. qmp_enter_<name> <args>
> 2. qmp_exit_<name> <ret> <succeeded>
> 
> (That's similar to how the syscalls Linux kernel trace events work.)
> 
> Scripts processing the trace can easily differentiate between enter
> (args) and exit (return value) events without parsing or keeping state
> to count the second event.
> 

OK, reasonable. This also makes patch 01 not needed.


-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] scripts/qapi-gen.py: add --add-trace-points option

[Vladimir Sementsov-Ogievskiy]

12.01.2022 03:38, John Snow wrote:
> On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> Add and option to generate trace points. We should generate both trace
>> points and trace-events files for further trace point code generation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   scripts/qapi/gen.py  | 13 ++++++++++---
>>   scripts/qapi/main.py | 10 +++++++---
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 995a97d2b8..605b3fe68a 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -251,7 +251,7 @@ def __init__(self,
>>           self._builtin_blurb = builtin_blurb
>>           self._pydoc = pydoc
>>           self._current_module: Optional[str] = None
>> -        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH, QAPIGen]] = {}
>>           self._main_module: Optional[str] = None
>>
>>       @property
>> @@ -264,6 +264,11 @@ def _genh(self) -> QAPIGenH:
>>           assert self._current_module is not None
>>           return self._module[self._current_module][1]
>>
>> +    @property
>> +    def _gent(self) -> QAPIGen:
>> +        assert self._current_module is not None
>> +        return self._module[self._current_module][2]
>> +
>>       @staticmethod
>>       def _module_dirname(name: str) -> str:
>>           if QAPISchemaModule.is_user_module(name):
>> @@ -293,7 +298,8 @@ def _add_module(self, name: str, blurb: str) -> None:
>>           basename = self._module_filename(self._what, name)
>>           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>>           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
>> -        self._module[name] = (genc, genh)
>> +        gent = QAPIGen(basename + '.trace-events')
>> +        self._module[name] = (genc, genh, gent)
>>           self._current_module = name
>>
>>       @contextmanager
>> @@ -304,11 +310,12 @@ def _temp_module(self, name: str) -> Iterator[None]:
>>           self._current_module = old_module
>>
>>       def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>> -        for name, (genc, genh) in self._module.items():
>> +        for name, (genc, genh, gent) in self._module.items():
>>               if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
>>                   continue
>>               genc.write(output_dir)
>>               genh.write(output_dir)
>> +            gent.write(output_dir)
>>
>>       def _begin_builtin_module(self) -> None:
>>           pass
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index f2ea6e0ce4..3adf0319cf 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_points: bool = False) -> None:
>>       """
>>       Generate C code for the given schema into the target directory.
>>
>> @@ -49,7 +50,7 @@ def generate(schema_file: str,
>>       schema = QAPISchema(schema_file)
>>       gen_types(schema, output_dir, prefix, builtins)
>>       gen_visit(schema, output_dir, prefix, builtins)
>> -    gen_commands(schema, output_dir, prefix)
>> +    gen_commands(schema, output_dir, prefix, add_trace_points)
>>       gen_events(schema, output_dir, prefix)
>>       gen_introspect(schema, output_dir, prefix, unmask)
>>
>> @@ -74,6 +75,8 @@ def main() -> int:
>>       parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>>                           dest='unmask',
>>                           help="expose non-ABI names in introspection")
>> +    parser.add_argument('--add-trace-points', action='store_true',
>> +                        help="add trace points to qmp marshals")
> 
> "Add trace events to generated marshaling functions." maybe?
> 
>>       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_points=args.add_trace_points)
>>       except QAPIError as err:
>>           print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
>>           return 1
>> --
>> 2.31.1
>>
> 
> I suppose the flag is so that non-QEMU invocations of the QAPI
> generator (for tests, etc) will compile correctly without tracepoint
> definitions, yeah?
> 

Yes, I had troubles with tests and some other code units, so I decided to do less to not fix things I'm not interested in) If needed it may be done in separate.

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument

[Kevin Wolf]

Am 17.01.2022 um 09:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 12.01.2022 13:50, Stefan Hajnoczi wrote:
> > On Tue, Jan 11, 2022 at 07:32:52PM -0500, John Snow wrote:
> > > On Tue, Jan 11, 2022 at 6:53 PM John Snow <jsnow@redhat.com> wrote:
> > > > 
> > > > On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy
> > > > <vsementsov@virtuozzo.com> wrote:
> > > > > 
> > > > > Add possibility to generate trace points for each qmp command.
> > > > > 
> > > > > We should generate both trace points and trace-events file, for further
> > > > > trace point code generation.
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > > ---
> > > > >   scripts/qapi/commands.py | 84 ++++++++++++++++++++++++++++++++++------
> > > > >   1 file changed, 73 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > > > > index 21001bbd6b..9691c11f96 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_points: bool) -> str:
> > > > >       ret = ''
> > > > > 
> > > > >       argstr = ''
> > > > > @@ -71,21 +72,65 @@ def gen_call(name: str,
> > > > >       if ret_type:
> > > > >           lhs = 'retval = '
> > > > > 
> > > > > -    ret = mcgen('''
> > > > > +    qmp_name = f'qmp_{c_name(name)}'
> > > > > +    upper = qmp_name.upper()
> > > > > +
> > > > > +    if add_trace_points:
> > > > > +        ret += mcgen('''
> > > > > +
> > > > > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > > > > +        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
> > > > > +        trace_%(qmp_name)s("", req_json->str);
> > > > > +    }
> > > > > +    ''',
> > > > > +                     upper=upper, qmp_name=qmp_name)
> > > > > +
> > > > > +    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) {
> > > > > +''')
> > > > > +
> > > > > +    if add_trace_points:
> > > > > +        ret += mcgen('''
> > > > > +        trace_%(qmp_name)s("FAIL: ", error_get_pretty(err));
> > > > > +''',
> > > > > +                     qmp_name=qmp_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_points:
> > > > > +        if ret_type:
> > > > > +            ret += mcgen('''
> > > > > +
> > > > > +    if (trace_event_get_state_backends(TRACE_%(upper)s)) {
> > > > > +        g_autoptr(GString) ret_json = qobject_to_json(*ret);
> > > > > +        trace_%(qmp_name)s("RET:", ret_json->str);
> > > > > +    }
> > > > > +    ''',
> > > > > +                     upper=upper, qmp_name=qmp_name)
> > > > > +        else:
> > > > > +            ret += mcgen('''
> > > > > +
> > > > > +    trace_%(qmp_name)s("SUCCESS", "");
> > > > > +    ''',
> > > > > +                         qmp_name=qmp_name)
> > > > > +
> > > > >       return ret
> > > > > 
> > > > > 
> > > > > @@ -122,10 +167,14 @@ def gen_marshal_decl(name: str) -> str:
> > > > >                    proto=build_marshal_proto(name))
> > > > > 
> > > > > 
> > > > > +def gen_trace(name: str) -> str:
> > > > > +    return f'qmp_{c_name(name)}(const char *tag, const char *json) "%s%s"\n'
> > > > > +
> > > > >   def gen_marshal(name: str,
> > > > >                   arg_type: Optional[QAPISchemaObjectType],
> > > > >                   boxed: bool,
> > > > > -                ret_type: Optional[QAPISchemaType]) -> str:
> > > > > +                ret_type: Optional[QAPISchemaType],
> > > > > +                add_trace_points: 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 +229,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_points)
> > > > > 
> > > > >       ret += mcgen('''
> > > > > 
> > > > > @@ -238,11 +287,12 @@ def gen_register_command(name: str,
> > > > > 
> > > > > 
> > > > >   class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> > > > > -    def __init__(self, prefix: str):
> > > > > +    def __init__(self, prefix: str, add_trace_points: bool):
> > > > >           super().__init__(
> > > > >               prefix, 'qapi-commands',
> > > > >               ' * Schema-defined QAPI/QMP commands', None, __doc__)
> > > > >           self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> > > > > +        self.add_trace_points = add_trace_points
> > > > > 
> > > > >       def _begin_user_module(self, name: str) -> None:
> > > > >           self._visited_ret_types[self._genc] = set()
> > > > > @@ -261,6 +311,15 @@ def _begin_user_module(self, name: str) -> None:
> > > > > 
> > > > >   ''',
> > > > >                                commands=commands, visit=visit))
> > > > > +
> > > > > +        if self.add_trace_points and c_name(commands) != 'qapi_commands':
> > > > > +            self._genc.add(mcgen('''
> > > > > +#include "trace/trace-qapi.h"
> > > > > +#include "qapi/qmp/qjson.h"
> > > > > +#include "trace/trace-%(nm)s_trace_events.h"
> > > > > +''',
> > > > > +                                 nm=c_name(commands)))
> > > > > +
> > > > >           self._genh.add(mcgen('''
> > > > >   #include "%(types)s.h"
> > > > > 
> > > > > @@ -322,7 +381,9 @@ def visit_command(self,
> > > > >           with ifcontext(ifcond, self._genh, self._genc):
> > > > >               self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> > > > >               self._genh.add(gen_marshal_decl(name))
> > > > > -            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> > > > > +            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
> > > > > +                                       self.add_trace_points))
> > > > > +            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 +393,8 @@ def visit_command(self,
> > > > > 
> > > > >   def gen_commands(schema: QAPISchema,
> > > > >                    output_dir: str,
> > > > > -                 prefix: str) -> None:
> > > > > -    vis = QAPISchemaGenCommandVisitor(prefix)
> > > > > +                 prefix: str,
> > > > > +                 add_trace_points: bool) -> None:
> > > > > +    vis = QAPISchemaGenCommandVisitor(prefix, add_trace_points)
> > > > >       schema.visit(vis)
> > > > >       vis.write(output_dir)
> > > > > --
> > > > > 2.31.1
> > > > > 
> > > > 
> > > > I looked at Stefan's feedback and I want to second his recommendation
> > > > to use _enter and _exit tracepoints, this closely resembles a lot of
> > > > temporary debugging code I've written for jobs/bitmaps over the years
> > > > and find the technique helpful.
> > > > 
> > > > --js
> > > > 
> > > > (as a tangent ...
> > > > 
> > > > I wish I had a much nicer way of doing C generation from Python, this
> > > > is so ugly. It's not your fault, of course. I'm just wondering if the
> > > > mailing list has any smarter ideas for future improvements to the
> > > > mcgen interface, because I find this type of code really hard to
> > > > review.)
> > > 
> > > Come to think of it, we could use a framework that was originally
> > > designed for HTML templating, but for C instead. Wait! Don't close the
> > > email yet, I have some funny things to write still!!
> > > 
> > > Downsides:
> > > - New template language
> > > - Complex
> > > 
> > > Pros:
> > > - Easier to read
> > > - Easier to review
> > > - Avoids reinventing the wheel
> > > - Doesn't even technically add a dependency, since Sphinx already
> > > relies on Jinja ...
> > > - *Extremely* funny
> > > 
> > > As an example, let's say we had a file
> > > "scripts/qapi/templates/qmp_marshal_output.c" that looked like this:
> > > ```
> > > static void qmp_marshal_output_{{c_name}}(
> > >      {{c_type}} ret_in,
> > >      QObject **ret_out,
> > >      Error **errp
> > > ) {
> > >      Visitor *v;
> > > 
> > >      v = qobject_output_visitor_new_qmp(ret_out);
> > >      if (visit_type_{{c_name}}(v, "unused", &ret_in, errp)) {
> > >          visit_complete(v, ret_out);
> > >      }
> > >      visit_free(v);
> > >      v = qapi_dealloc_visitor_new();
> > >      visit_type_{{c_name}}(v, "unused", &ret_in, NULL);
> > >      visit_free(v);
> > > }
> > > ```
> > > 
> > > We could use Jinja to process it from Python like this:
> > > 
> > > ```
> > > import os
> > > from jinja2 import PackageLoader, Environment, FileSystemLoader
> > > 
> > > env = Environment(loader = FileSystemLoader('./templates/'))
> > > template = env.get_template("qmp_marshal_output.c")
> > > rendered = cgen(template.render(
> > >      c_name = "foo",
> > >      c_type = "int",
> > > ))
> > > 
> > > print(rendered)
> > > ```
> > > 
> > > You'd get output like this:
> > > 
> > > ```
> > > static void qmp_marshal_output_foo(
> > >      int ret_in,
> > >      QObject **ret_out,
> > >      Error **errp
> > > ) {
> > >      Visitor *v;
> > > 
> > >      v = qobject_output_visitor_new_qmp(ret_out);
> > >      if (visit_type_foo(v, "unused", &ret_in, errp)) {
> > >          visit_complete(v, ret_out);
> > >      }
> > >      visit_free(v);
> > >      v = qapi_dealloc_visitor_new();
> > >      visit_type_foo(v, "unused", &ret_in, NULL);
> > >      visit_free(v);
> > > ```
> > > 
> > > Jinja also supports conditionals and some other logic constructs, so
> > > it'd *probably* apply to most templates we'd want to write, though
> > > admittedly I haven't thought through if it'd actually work everywhere
> > > we'd need it, and I am mostly having a laugh.
> > > 
> > > ...OK, back to work!
> > 
> > I agree that mcgen string formatting is very hard to review.
> > 
> > I'm not sure if it's possible to separate the C templates into large
> > chunks that are easy to read though. If there are many small C templates
> > then it's hard to review and I think that's the core problem, not the
> > string formatting/expansion mechanism (%(var)s vs Python {var} vs jinja2
> > {{var}}).
> > 
> > If we could stick to Python standard library features instead of jinja2
> > that would be nice because while jinja2 is powerful and widely-used,
> > it's probably a bit too powerful and adds complexity/boilerplate that
> > could be overkill in this situation.
> > 
> > Stefan
> 
> I think that conversion to Python f-strings would be a good start: not
> very invasive, no extra dependencies, but may probably solve 60-80 %
> of the problem.

It doesn't really. As I understand it, the main problem mcgen() solves is
outputting correctly indented C code when you don't have a single
string, but the code has to be split across multiple strings because of
conditions and loops.

In general, f-strings are a little bit nicer than the formatting to be
used with the % operator, but you'd still keep all those mcgen() calls
with code between them. They just wouldn't need **kwds any more.

Of course, in the specific case of outputting C code, f-strings would
actually be terrible because you'd have to double all braces, which tend
to occur a lot in C code. So in this case, they probably aren't a little
bit nicer, but a little bit uglier.

Kevin