Re: [Qemu-devel] [PATCH v6 10/15] qapi/commands: add #if conditions to commands

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Wrap generated code with #if/#endif using an 'ifcontext' on
>> QAPIGenCSnippet objects.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  scripts/qapi/commands.py | 21 ++++++++++++---------
>>  tests/test-qmp-cmds.c    |  5 +++--
>>  2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index dcc03c7859..72749c7fc5 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -239,7 +239,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>>          QAPISchemaModularCVisitor.__init__(
>>              self, prefix, 'qapi-commands',
>>              ' * Schema-defined QAPI/QMP commands', __doc__)
>> -        self._regy = ''
>> +        self._regy = QAPIGenCCode()
>>          self._visited_ret_types = {}
>>  
>>      def _begin_module(self, name):
>> @@ -275,20 +275,23 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>>  ''',
>>                         c_prefix=c_name(self._prefix, protect=False)))
>> -        genc.add(gen_registry(self._regy, self._prefix))
>> +        genc.add(gen_registry(self._regy.get_content(), self._prefix))
>>  
>>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>>                        success_response, boxed, allow_oob, allow_preconfig):
>>          if not gen:
>>              return
>> -        self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
>> -        if ret_type and ret_type not in self._visited_ret_types[self._genc]:
>> +        if ret_type and \
>> +           ret_type not in self._visited_ret_types[self._genc]:
>
> PEP8 prefers parenthesis over backslash.  Can touch this up when I
> apply.
>
>>              self._visited_ret_types[self._genc].add(ret_type)
>> -            self._genc.add(gen_marshal_output(ret_type))
>> -        self._genh.add(gen_marshal_decl(name))
>> -        self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
>> -        self._regy += gen_register_command(name, success_response, allow_oob,
>> -                                           allow_preconfig)
>> +            with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
>> +                self._genc.add(gen_marshal_output(ret_type))
>> +        with ifcontext(ifcond, self._genh, self._genc, self._regy):
>> +            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._regy.add(gen_register_command(name, success_response,
>> +                                                allow_oob, allow_preconfig))
>
> Okay, now it falls apart differently :)  Let's step through the code
> real slow.
>
> The first time we visit a command C returning type T...
>
>         if ret_type and \
>            ret_type not in self._visited_ret_types[self._genc]:
>             self._visited_ret_types[self._genc].add(ret_type)
>             with ifcontext(ret_type.ifcond, self._genh, self._genc, self._regy):
>                 self._genc.add(gen_marshal_output(ret_type))
>
> ... we generate qmp_marshal_output_T() wrapped in T's condition.
>
>         with ifcontext(ifcond, self._genh, self._genc, self._regy):
>             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._regy.add(gen_register_command(name, success_response,
>                                                 allow_oob, allow_preconfig))
>
> We generate qmp_marshal_C() wrapped in C's condition.  This is what
> calls qmp_marshal_output_T().
>
> If T is a user-defined type, the user is responsible for making this
> work, i.e. to make T's condition the conjunction of the T-returning
> commands' conditions.
>
> If T is a built-in type, this isn't possible: the qmp_marshal_output_T()
> will be generated unconditionally.
>
> I append a crude patch to provide test coverage (crude because it
> removes coverage of another case).  With it applied, make check dies
> with
>
>     tests/test-qapi-commands.c:470:13: warning: ‘qmp_marshal_output_str’ defined but not used [-Wunused-function]
>
> I think the issue is obscure enough to justify postponing a proper fix,
> and just add a FIXME here now.  I can do that when I apply.

With the PEP8 tweak and a suitable FIXME
Reviewed-by: Markus Armbruster <armbru@redhat.com>