qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v4 2/3] scripts/qapi/commands: gen_commands(): add add_trace_events arg
Date: Tue, 25 Jan 2022 13:20:05 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

25.01.2022 13:02, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

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

Now implement that possibility in gen_commands() function.

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

Neglects to explain why it needs to be optional.

Suggest

   qapi/commands: Optionally generate trace for QMP commands

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


Thanks, sounds good.


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

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

Before the patch, we generate

     error_propagate(errp, err);

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

     if (err) {
         goto out;
     }

After the patch, we always generate

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

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

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

     trace_qmp_exit_FOO("{}", true);

Okay.

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

Missing:

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

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

--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]