On Tue, Jan 11, 2022 at 6:53 PM John Snow <js...@redhat.com> wrote: > > On Thu, Dec 23, 2021 at 6:08 AM Vladimir Sementsov-Ogievskiy > <vsement...@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 <vsement...@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