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