qemu-block
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 2/4] scripts/qapi/commands: gen_commands(): add add_trace_points argument
Date: Mon, 17 Jan 2022 11:45:26 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]