| Message ID | 20211223110756.699148-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace qmp commands | expand |
On Thu, Dec 23, 2021 at 12:07:54PM +0100, Vladimir Sementsov-Ogievskiy 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: Please use the term "trace events" instead of "trace points". That's the term that docs/devel/tracing.rst uses. > @@ -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' This trace event is emitted in 3 different ways: 1. For arguments before calling a QMP command. 2. For the error message when the QMP command fails. 3. For the return value when a QMP command succeeds. This makes parsing the trace akward because you get two events in succession for a single call and they both have the same name. Please generate 2 trace events: 1. qmp_enter_<name> <args> 2. qmp_exit_<name> <ret> <succeeded> (That's similar to how the syscalls Linux kernel trace events work.) Scripts processing the trace can easily differentiate between enter (args) and exit (return value) events without parsing or keeping state to count the second event. Thanks, Stefan
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.)
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
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
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).
10.01.2022 19:22, Stefan Hajnoczi wrote: > On Thu, Dec 23, 2021 at 12:07:54PM +0100, Vladimir Sementsov-Ogievskiy 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: > > Please use the term "trace events" instead of "trace points". That's the > term that docs/devel/tracing.rst uses. > >> @@ -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' > > This trace event is emitted in 3 different ways: > 1. For arguments before calling a QMP command. > 2. For the error message when the QMP command fails. > 3. For the return value when a QMP command succeeds. > > This makes parsing the trace akward because you get two events in > succession for a single call and they both have the same name. > > Please generate 2 trace events: > 1. qmp_enter_<name> <args> > 2. qmp_exit_<name> <ret> <succeeded> > > (That's similar to how the syscalls Linux kernel trace events work.) > > Scripts processing the trace can easily differentiate between enter > (args) and exit (return value) events without parsing or keeping state > to count the second event. > OK, reasonable. This also makes patch 01 not needed.
Am 17.01.2022 um 09:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > 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. It doesn't really. As I understand it, the main problem mcgen() solves is outputting correctly indented C code when you don't have a single string, but the code has to be split across multiple strings because of conditions and loops. In general, f-strings are a little bit nicer than the formatting to be used with the % operator, but you'd still keep all those mcgen() calls with code between them. They just wouldn't need **kwds any more. Of course, in the specific case of outputting C code, f-strings would actually be terrible because you'd have to double all braces, which tend to occur a lot in C code. So in this case, they probably aren't a little bit nicer, but a little bit uglier. Kevin
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)
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(-)