Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The generated code doesn't quite handle the conditional arguments.
> For example, 'bar' in 'test-if-cmd' is not correctly surrounded by #if
> conditions. See generated code in qmp_marshal_test_if_cmd().
>
> Note that if there are multiple optional arguments at the last position,
> there might be compilation issues due to extra comas. I left an assert
> and FIXME for later.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/commands.py                |  4 ++++
>  scripts/qapi/gen.py                     | 19 ++++++++++++++-----
>  scripts/qapi/visit.py                   |  2 ++
>  tests/qapi-schema/qapi-schema-test.json |  3 ++-
>  4 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 79c5e5c3a9..07997d1586 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -64,9 +64,13 @@ def gen_call(name: str,
>      elif arg_type:
>          assert not arg_type.variants
>          for memb in arg_type.members:
> +            if memb.ifcond.is_present():
> +                argstr += '\n' + memb.ifcond.gen_if()
>              if memb.need_has():
>                  argstr += 'arg.has_%s, ' % c_name(memb.name)
>              argstr += 'arg.%s, ' % c_name(memb.name)
> +            if memb.ifcond.is_present():
> +                argstr += '\n' + memb.ifcond.gen_endif()
>  
>      lhs = ''
>      if ret_type:

@argstr is emitted further down:

       %(lhs)sqmp_%(name)s(%(args)s&err);
   ''',
                    name=name, args=argstr, lhs=lhs)

       ret += mcgen('''
       if (err) {
   ''')

Before the patch, @argstr contains no newlines.  Works.

After the patch, it may contain newlines, and if it does, intentation is
messed up.  For instance, in the code generated for
qapi-schema-test.json:

        retval = qmp_test_if_cmd(arg.foo, 
    #if defined(TEST_IF_CMD_BAR)
    arg.bar, 
    #endif /* defined(TEST_IF_CMD_BAR) */
    &err);

Strings interpolated into the mcgen() argument should not contain
newlines.  I'm afraid you have to rewrite the code emitting the call.

> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index b5a8d03e8e..ba57e72c9b 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -111,22 +111,31 @@ def build_params(arg_type: 
> Optional[QAPISchemaObjectType],
>                   boxed: bool,
>                   extra: Optional[str] = None) -> str:
>      ret = ''
> -    sep = ''
>      if boxed:
>          assert arg_type
>          ret += '%s arg' % arg_type.c_param_type()
> -        sep = ', '
> +        if extra:
> +            ret += ', '
>      elif arg_type:
>          assert not arg_type.variants
> +        n = 0
>          for memb in arg_type.members:
> -            ret += sep
> -            sep = ', '
> +            n += 1
> +            if memb.ifcond.is_present():
> +                ret += '\n' + memb.ifcond.gen_if()
>              if memb.need_has():
>                  ret += 'bool has_%s, ' % c_name(memb.name)
>              ret += '%s %s' % (memb.type.c_param_type(),
>                                c_name(memb.name))
> +            if extra or n != len(arg_type.members):
> +                ret += ', '
> +            else:
> +                # FIXME: optional last argument may break compilation
> +                assert not memb.ifcond.is_present()

Does the assertion guard against the C compilation failure?

Is it possible to write schema code that triggers it?

> +            if memb.ifcond.is_present():
> +                ret += '\n' + memb.ifcond.gen_endif()
>      if extra:
> -        ret += sep + extra
> +        ret += extra
>      return ret if ret else 'void'
>  
>  

Same newline issue as in gen_call().  Generated code:

    UserDefThree *qmp_test_if_cmd(TestIfStruct *foo, 
    #if defined(TEST_IF_CMD_BAR)
    TestIfEnum bar, 
    #endif /* defined(TEST_IF_CMD_BAR) */
    Error **errp);

> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 26a584ee4c..c56ea4d724 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -74,11 +74,13 @@ def gen_visit_object_members(name: str,
>      sep = ''
>      for memb in members:
>          if memb.optional and not memb.need_has():
> +            ret += memb.ifcond.gen_if()
>              ret += mcgen('''
>      bool has_%(c_name)s = !!obj->%(c_name)s;
>  ''',
>                           c_name=c_name(memb.name))
>              sep = '\n'
> +            ret += memb.ifcond.gen_endif()
>      ret += sep
>  
>      if base:

This hunk has no effect on the code generated for our schemas as far as
I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
am I confused?

> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index ba7302f42b..baa4e69f63 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -258,7 +258,8 @@
>  
>  { 'event': 'TEST_IF_EVENT',
>    'data': { 'foo': 'TestIfStruct',
> -            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
> +            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' },
> +            'baz': 'int' },
>    'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
>  
>  { 'event': 'TEST_IF_EVENT2', 'data': {},