diff mbox series

[v6,3/7] qapi/commands: Optionally generate trace for QMP commands

Message ID 20220126161130.3240892-4-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series trace qmp commands | expand

Commit Message

Vladimir Sementsov-Ogievskiy Jan. 26, 2022, 4:11 p.m. UTC
Add trace generation disabled by default and new option --gen-trace to
enable it.  The next commit will enable it for qapi/, but not for qga/
and tests/.  Making it work for the latter two would involve some Meson
hackery to ensure we generate the trace-events files before trace-tool
uses them.  Since we don't actually support tracing there, we'll bypass
that problem.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/qapi/commands.py | 91 +++++++++++++++++++++++++++++++++++-----
 scripts/qapi/main.py     | 14 +++++--
 2 files changed, 91 insertions(+), 14 deletions(-)

Comments

Markus Armbruster Jan. 27, 2022, 7:24 a.m. UTC | #1
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Add trace generation disabled by default and new option --gen-trace to
> enable it.  The next commit will enable it for qapi/, but not for qga/
> and tests/.  Making it work for the latter two would involve some Meson
> hackery to ensure we generate the trace-events files before trace-tool
> uses them.  Since we don't actually support tracing there, we'll bypass
> that problem.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  scripts/qapi/commands.py | 91 +++++++++++++++++++++++++++++++++++-----
>  scripts/qapi/main.py     | 14 +++++--
>  2 files changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 17e5ed2414..fa90b6246b 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py

[...]

> @@ -265,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>  
>  ''',
>                               commands=commands, visit=visit))
> +
> +        if self._gen_tracing and commands != 'qapi-commands':
> +            self._genc.add(mcgen('''
> +#include "trace/trace-qapi.h"

I believe this include is superfluous.

> +#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"
>  

[...]
Vladimir Sementsov-Ogievskiy Jan. 27, 2022, 9:42 a.m. UTC | #2
27.01.2022 10:24, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Add trace generation disabled by default and new option --gen-trace to
>> enable it.  The next commit will enable it for qapi/, but not for qga/
>> and tests/.  Making it work for the latter two would involve some Meson
>> hackery to ensure we generate the trace-events files before trace-tool
>> uses them.  Since we don't actually support tracing there, we'll bypass
>> that problem.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   scripts/qapi/commands.py | 91 +++++++++++++++++++++++++++++++++++-----
>>   scripts/qapi/main.py     | 14 +++++--
>>   2 files changed, 91 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 17e5ed2414..fa90b6246b 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
> 
> [...]
> 
>> @@ -265,6 +319,17 @@ def _begin_user_module(self, name: str) -> None:
>>   
>>   ''',
>>                                commands=commands, visit=visit))
>> +
>> +        if self._gen_tracing and commands != 'qapi-commands':
>> +            self._genc.add(mcgen('''
>> +#include "trace/trace-qapi.h"
> 
> I believe this include is superfluous.

No objections to drop it

> 
>> +#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"
>>   
> 
> [...]
>
diff mbox series

Patch

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 17e5ed2414..fa90b6246b 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -53,7 +53,8 @@  def gen_command_decl(name: str,
 def gen_call(name: str,
              arg_type: Optional[QAPISchemaObjectType],
              boxed: bool,
-             ret_type: Optional[QAPISchemaType]) -> str:
+             ret_type: Optional[QAPISchemaType],
+             gen_tracing: bool) -> str:
     ret = ''
 
     argstr = ''
@@ -71,14 +72,37 @@  def gen_call(name: str,
     if ret_type:
         lhs = 'retval = '
 
-    ret = mcgen('''
+    name = c_name(name)
+    upper = name.upper()
 
-    %(lhs)sqmp_%(c_name)s(%(args)s&err);
+    if gen_tracing:
+        ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_QMP_ENTER_%(upper)s)) {
+        g_autoptr(GString) req_json = qobject_to_json(QOBJECT(args));
+
+        trace_qmp_enter_%(name)s(req_json->str);
+    }
+    ''',
+                     upper=upper, name=name)
+
+    ret += mcgen('''
+
+    %(lhs)sqmp_%(name)s(%(args)s&err);
 ''',
-                c_name=c_name(name), args=argstr, lhs=lhs)
+                 name=name, args=argstr, lhs=lhs)
 
     ret += mcgen('''
     if (err) {
+''')
+
+    if gen_tracing:
+        ret += mcgen('''
+        trace_qmp_exit_%(name)s(error_get_pretty(err), false);
+''',
+                     name=name)
+
+    ret += mcgen('''
         error_propagate(errp, err);
         goto out;
     }
@@ -90,6 +114,25 @@  def gen_call(name: str,
     qmp_marshal_output_%(c_name)s(retval, ret, errp);
 ''',
                      c_name=ret_type.c_name())
+
+    if gen_tracing:
+        if ret_type:
+            ret += mcgen('''
+
+    if (trace_event_get_state_backends(TRACE_QMP_EXIT_%(upper)s)) {
+        g_autoptr(GString) ret_json = qobject_to_json(*ret);
+
+        trace_qmp_exit_%(name)s(ret_json->str, true);
+    }
+    ''',
+                         upper=upper, name=name)
+        else:
+            ret += mcgen('''
+
+    trace_qmp_exit_%(name)s("{}", true);
+    ''',
+                         name=name)
+
     return ret
 
 
@@ -126,10 +169,19 @@  def gen_marshal_decl(name: str) -> str:
                  proto=build_marshal_proto(name))
 
 
+def gen_trace(name: str) -> str:
+    return mcgen('''
+qmp_enter_%(name)s(const char *json) "%%s"
+qmp_exit_%(name)s(const char *result, bool succeeded) "%%s %%d"
+''',
+                 name=c_name(name))
+
+
 def gen_marshal(name: str,
                 arg_type: Optional[QAPISchemaObjectType],
                 boxed: bool,
-                ret_type: Optional[QAPISchemaType]) -> str:
+                ret_type: Optional[QAPISchemaType],
+                gen_tracing: bool) -> str:
     have_args = boxed or (arg_type and not arg_type.is_empty())
     if have_args:
         assert arg_type is not None
@@ -184,7 +236,7 @@  def gen_marshal(name: str,
     }
 ''')
 
-    ret += gen_call(name, arg_type, boxed, ret_type)
+    ret += gen_call(name, arg_type, boxed, ret_type, gen_tracing)
 
     ret += mcgen('''
 
@@ -242,11 +294,13 @@  def gen_register_command(name: str,
 
 
 class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
-    def __init__(self, prefix: str):
+    def __init__(self, prefix: str, gen_tracing: bool):
         super().__init__(
             prefix, 'qapi-commands',
-            ' * Schema-defined QAPI/QMP commands', None, __doc__)
+            ' * Schema-defined QAPI/QMP commands', None, __doc__,
+            gen_tracing=gen_tracing)
         self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
+        self._gen_tracing = gen_tracing
 
     def _begin_user_module(self, name: str) -> None:
         self._visited_ret_types[self._genc] = set()
@@ -265,6 +319,17 @@  def _begin_user_module(self, name: str) -> None:
 
 ''',
                              commands=commands, visit=visit))
+
+        if self._gen_tracing and commands != 'qapi-commands':
+            self._genc.add(mcgen('''
+#include "trace/trace-qapi.h"
+#include "qapi/qmp/qjson.h"
+#include "trace/trace-%(nm)s_trace_events.h"
+''',
+                                 nm=c_name(commands, protect=False)))
+            # We use c_name(commands, protect=False) to turn '-' into '_', to
+            # match .underscorify() in trace/meson.build
+
         self._genh.add(mcgen('''
 #include "%(types)s.h"
 
@@ -326,7 +391,10 @@  def visit_command(self,
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
             self._genh.add(gen_marshal_decl(name))
-            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
+            self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,
+                                       self._gen_tracing))
+            if self._gen_tracing:
+                self._gen_trace_events.add(gen_trace(name))
         with self._temp_module('./init'):
             with ifcontext(ifcond, self._genh, self._genc):
                 self._genc.add(gen_register_command(
@@ -336,7 +404,8 @@  def visit_command(self,
 
 def gen_commands(schema: QAPISchema,
                  output_dir: str,
-                 prefix: str) -> None:
-    vis = QAPISchemaGenCommandVisitor(prefix)
+                 prefix: str,
+                 gen_tracing: bool) -> None:
+    vis = QAPISchemaGenCommandVisitor(prefix, gen_tracing)
     schema.visit(vis)
     vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..687d408aba 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -32,7 +32,8 @@  def generate(schema_file: str,
              output_dir: str,
              prefix: str,
              unmask: bool = False,
-             builtins: bool = False) -> None:
+             builtins: bool = False,
+             gen_tracing: bool = False) -> None:
     """
     Generate C code for the given schema into the target directory.
 
@@ -49,7 +50,7 @@  def generate(schema_file: str,
     schema = QAPISchema(schema_file)
     gen_types(schema, output_dir, prefix, builtins)
     gen_visit(schema, output_dir, prefix, builtins)
-    gen_commands(schema, output_dir, prefix)
+    gen_commands(schema, output_dir, prefix, gen_tracing)
     gen_events(schema, output_dir, prefix)
     gen_introspect(schema, output_dir, prefix, unmask)
 
@@ -74,6 +75,12 @@  def main() -> int:
     parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
                         dest='unmask',
                         help="expose non-ABI names in introspection")
+
+    # Option --gen-trace exists so we can avoid solving build system
+    # problems.  TODO Drop it when we no longer need it.
+    parser.add_argument('--gen-trace', action='store_true',
+                        help="add trace events to qmp marshals")
+
     parser.add_argument('schema', action='store')
     args = parser.parse_args()
 
@@ -88,7 +95,8 @@  def main() -> int:
                  output_dir=args.output_dir,
                  prefix=args.prefix,
                  unmask=args.unmask,
-                 builtins=args.builtins)
+                 builtins=args.builtins,
+                 gen_tracing=args.gen_trace)
     except QAPIError as err:
         print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
         return 1