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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7393 bytes --]

Hi Markus

On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
wrote:

> 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.
>

Why it should not contain newlines?

What are you asking exactly? that the caller be changed? (this does not
work well if there are multiple optional arguments..)

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


>
> > 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?
>

Yes


>
> Is it possible to write schema code that triggers it?
>

Yes, the one we have for TEST_IF_EVENT for example:

{ 'event': 'TEST_IF_EVENT',
  'data': { 'foo': 'TestIfStruct',
            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },

produces:

void qapi_event_send_test_if_event(TestIfStruct *foo,
#if defined(TEST_IF_EVT_BAR)
TestIfEnumList *bar,
#endif /* defined(TEST_IF_EVT_BAR) */
);

Which will fail to compile if TEST_IF_EVT_BAR is undefined.

So I would rather assert that we don't introduce such a schema, until we
fix the code generator. Or we acknowledge the limitation, and treat it as a
schema error. Other ideas?


> > +            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?
>
>
Right, we could change the test this way to exercise it:

--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -250,7 +250,7 @@
 { 'command': 'test-if-cmd',
   'data': {
     'foo': 'TestIfStruct',
-    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
+    '*bar': { 'type': 'TestIfStruct', 'if': 'TEST_IF_STRUCT' } },
   'returns': 'UserDefThree',


> > 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': {},
>
>

[-- Attachment #2: Type: text/html, Size: 10988 bytes --]