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