Hi Markus
On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <arm...@redhat.com>
wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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': {},
>
>
