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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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