On Tue, Jan 11, 2022 at 07:32:52PM -0500, John Snow wrote: > 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!
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
signature.asc
Description: PGP signature