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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Wrap generated code with #if/#endif using an 'ifcontext' on
> QAPIGenCSnippet objects.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  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.

>  
>  
>  def gen_commands(schema, output_dir, prefix):
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index 840530b84c..bd27353908 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -12,12 +12,13 @@
>  
>  static QmpCommandList qmp_commands;
>  
> -/* #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) */
> +#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
>  UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
> +void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)

Whoops!

Not caught by make check since it compiles with none of the conditionals
defined.  Improvement welcome, but not necessary to get this series in.
I can fix the editing accident when I apply.

>  {
>      return NULL;
>  }
> -/* #endif */
> +#endif
>  
>  UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
>  {


diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 16209b57b3..4dd1ce3703 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -8,7 +8,8 @@
     # Commands allowed to return a non-dictionary:
     'returns-whitelist': [
         'guest-get-time',
-        'guest-sync' ] } }
+        'guest-sync',
+        'TestIfCmd' ] } }
 
 { 'struct': 'TestStruct',
   'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } }
@@ -56,9 +57,6 @@
   'data': { 'string0': 'str',
             'dict1': 'UserDefTwoDict' } }
 
-{ 'struct': 'UserDefThree',
-  'data': { 'string0': 'str' } }
-
 # dummy struct to force generation of array types not otherwise mentioned
 { 'struct': 'ForceArrays',
   'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'],
@@ -212,10 +210,8 @@
   'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
 
 { 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
-  'returns': 'UserDefThree',
+  'returns': 'str',
   'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
 
-{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
-
 { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
   'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index 0da92455da..aafa40c226 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -36,8 +36,6 @@ object UserDefTwoDict
 object UserDefTwo
     member string0: str optional=False
     member dict1: UserDefTwoDict optional=False
-object UserDefThree
-    member string0: str optional=False
 object ForceArrays
     member unused1: UserDefOneList optional=False
     member unused2: UserDefTwoList optional=False
@@ -257,11 +255,9 @@ alternate TestIfAlternate
 object q_obj_TestIfCmd-arg
     member foo: TestIfStruct optional=False
     if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
+command TestIfCmd q_obj_TestIfCmd-arg -> str
    gen=True success_response=True boxed=False oob=False preconfig=False
     if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestCmdReturnDefThree None -> UserDefThree
-   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_TestIfEvent-arg
     member foo: TestIfStruct optional=False
     if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index bd27353908..6ba73fd53c 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -13,18 +13,12 @@
 static QmpCommandList qmp_commands;
 
 #if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD)
-UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
-void qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
+char *qmp_TestIfCmd(TestIfStruct *foo, Error **errp)
 {
     return NULL;
 }
 #endif
 
-UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
-{
-    return NULL;
-}
-
 void qmp_user_def_cmd(Error **errp)
 {
 }