| Message ID | 20230207142535.1153722-8-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Teach 'getfd' QMP command to import win32 sockets | expand |
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> Fails "make check" for me: 2/2 qemu:qapi-schema+qapi-frontend / QAPI schema regression tests FAIL 0.09s exit status 1 >>> MALLOC_PERTURB_=241 PYTHONPATH=/work/armbru/qemu/scripts /usr/bin/python3 [...] ――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――― stdout: --- /work/armbru/qemu/bld-clang/../tests/qapi-schema/qapi-schema-test.out +++ @@ -297,6 +297,7 @@ member foo: TestIfStruct optional=False member bar: TestIfEnumList optional=False if TEST_IF_EVT_BAR + member baz: int optional=False if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']} event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg boxed=False stderr: qapi-schema-test FAIL
Hi Markus On Thu, Feb 9, 2023 at 4:42 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> > > Fails "make check" for me: > > 2/2 qemu:qapi-schema+qapi-frontend / QAPI schema regression tests > FAIL 0.09s exit status 1 > >>> MALLOC_PERTURB_=241 PYTHONPATH=/work/armbru/qemu/scripts > /usr/bin/python3 [...] > ――――――――――――――――――――――――――――――――――――― ✀ > ――――――――――――――――――――――――――――――――――――― > stdout: > --- /work/armbru/qemu/bld-clang/../tests/qapi-schema/qapi-schema-test.out > +++ > @@ -297,6 +297,7 @@ > member foo: TestIfStruct optional=False > member bar: TestIfEnumList optional=False > if TEST_IF_EVT_BAR > + member baz: int optional=False > if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']} > event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg > boxed=False > stderr: > qapi-schema-test FAIL > This is trivially fixed. Can you review the patch, and in particular comment on the FIXME left, whether it is acceptable? thanks
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': {},
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': {}, > >
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > 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? They mess up indentation. I think. It's been a while... All I really know for sure is that the generated code's indentation is messed up right there. > 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) */ I'm asking for better indentation. In handwritten code, we'd do retval = qmp_test_if_cmd(arg.foo, #if defined(TEST_IF_CMD_BAR) arg.bar, #endif /* defined(TEST_IF_CMD_BAR) */ &err); Keeping track of how far to indent the arguments is bothersome in the generator, though. Perhaps we could create infrastructure to make it not bothersome, but I'm not asking for that. Something like this should be good enough: retval = qmp_test_if_cmd(arg.foo, #if defined(TEST_IF_CMD_BAR) arg.bar, #endif /* defined(TEST_IF_CMD_BAR) */ &err); I.e. indent to the function call and then some. >> > 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' } }, This is the one you put in qapi-schema-test.json less the last parameter, so that the conditional parameter becomes the last one. > 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. I think it'll fail to compile always, because the parameter list has a trailing comma regardless of TEST_IF_EVT_BAR. > 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? Yes: throw an error. Assertions are for programming errors. This isn't a programming error, it's a limitation of the current implementation. How hard would it be to lift the limitation? >> > + 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', Please exercise it :) >> > 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': {}, >> >>
Hi On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote: > > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > 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? > > They mess up indentation. I think. It's been a while... All I really > know for sure is that the generated code's indentation is messed up > right there. > > > 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) */ > > I'm asking for better indentation. In handwritten code, we'd do > > retval = qmp_test_if_cmd(arg.foo, > #if defined(TEST_IF_CMD_BAR) > arg.bar, > #endif /* defined(TEST_IF_CMD_BAR) */ > &err); > > Keeping track of how far to indent the arguments is bothersome in the > generator, though. Perhaps we could create infrastructure to make it > not bothersome, but I'm not asking for that. Something like this should > be good enough: > > retval = qmp_test_if_cmd(arg.foo, > #if defined(TEST_IF_CMD_BAR) > arg.bar, > #endif /* defined(TEST_IF_CMD_BAR) */ > &err); > > I.e. indent to the function call and then some. ok, I improved the indentation a bit. However, I think it would be simpler, and better, if we piped the generated code to clang-format (when available). I made a simple patch for that too. > > >> > 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' } }, > > This is the one you put in qapi-schema-test.json less the last > parameter, so that the conditional parameter becomes the last one. > > > 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. > > I think it'll fail to compile always, because the parameter list has a > trailing comma regardless of TEST_IF_EVT_BAR. Yes, I think I hand-wrote that example, the actual generator does not leave a trailing comma here. > > > 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? > > Yes: throw an error. Assertions are for programming errors. This isn't > a programming error, it's a limitation of the current implementation. > > How hard would it be to lift the limitation? Taking this as a problematic example: void function(first, #ifdef A a, #endif #ifdef B b #endif ) I think it would mean that we would have to pass arguments as a structure, as they don't have the limitation of trailing coma in initializers. That would not be idiomatic C though, and we would need to refactor a lot of code.. Another option is to always pass a dummy last argument? :) void command(first, #ifdef A a, #endif #ifdef B b, #endif dummy) > > >> > + 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', > > Please exercise it :) ok > > >> > 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': {}, > >> > >> > > thanks
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >> >> > 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? >> >> They mess up indentation. I think. It's been a while... All I really >> know for sure is that the generated code's indentation is messed up >> right there. >> >> > 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) */ >> >> I'm asking for better indentation. In handwritten code, we'd do >> >> retval = qmp_test_if_cmd(arg.foo, >> #if defined(TEST_IF_CMD_BAR) >> arg.bar, >> #endif /* defined(TEST_IF_CMD_BAR) */ >> &err); >> >> Keeping track of how far to indent the arguments is bothersome in the >> generator, though. Perhaps we could create infrastructure to make it >> not bothersome, but I'm not asking for that. Something like this should >> be good enough: >> >> retval = qmp_test_if_cmd(arg.foo, >> #if defined(TEST_IF_CMD_BAR) >> arg.bar, >> #endif /* defined(TEST_IF_CMD_BAR) */ >> &err); >> >> I.e. indent to the function call and then some. > > ok, I improved the indentation a bit. > > However, I think it would be simpler, and better, if we piped the > generated code to clang-format (when available). I made a simple patch > for that too. Piping through indent or clang-format may well give us neater results for less effort. We might want to dumb down generator code then. >> >> > 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' } }, >> >> This is the one you put in qapi-schema-test.json less the last >> parameter, so that the conditional parameter becomes the last one. >> >> > 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. >> >> I think it'll fail to compile always, because the parameter list has a >> trailing comma regardless of TEST_IF_EVT_BAR. > > Yes, I think I hand-wrote that example, the actual generator does not > leave a trailing comma here. > >> >> > 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? >> >> Yes: throw an error. Assertions are for programming errors. This isn't >> a programming error, it's a limitation of the current implementation. >> >> How hard would it be to lift the limitation? > > Taking this as a problematic example: > > void function(first, > #ifdef A > a, > #endif > #ifdef B > b > #endif > ) > > I think it would mean that we would have to pass arguments as a > structure, as they don't have the limitation of trailing coma in > initializers. That would not be idiomatic C though, and we would need > to refactor a lot of code.. > > Another option is to always pass a dummy last argument? :) > > void command(first, > #ifdef A > a, > #endif > #ifdef B > b, > #endif > dummy) Yet another option: void command(first #ifdef A , a #endif #ifdef B , b #endif ) [...]
Hi On Wed, Feb 22, 2023 at 2:23 PM Markus Armbruster <armbru@redhat.com> wrote: > > Another option is to always pass a dummy last argument? :) > > > > void command(first, > > #ifdef A > > a, > > #endif > > #ifdef B > > b, > > #endif > > dummy) > > Yet another option: > > void command(first > #ifdef A > , a > #endif > #ifdef B > , b > #endif > ) > > [...] > Since I think we always have a first argument, that might be indeed the best solution. I'll try that. thanks
Hi On Wed, Feb 22, 2023 at 2:29 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Wed, Feb 22, 2023 at 2:23 PM Markus Armbruster <armbru@redhat.com> wrote: > > > Another option is to always pass a dummy last argument? :) > > > > > > void command(first, > > > #ifdef A > > > a, > > > #endif > > > #ifdef B > > > b, > > > #endif > > > dummy) > > > > Yet another option: > > > > void command(first > > #ifdef A > > , a > > #endif > > #ifdef B > > , b > > #endif > > ) > > > > [...] > > > > Since I think we always have a first argument, that might be indeed > the best solution. I'll try that. thanks > Actually, this is just moving the problem to the first argument. (and it also breaks clang-format, which doesn't have a allow-leading-commas option or similar). So I'll just add an error when using conditional fields/args last.
On Tue, Feb 07, 2023 at 06:25:32PM +0400, marcandre.lureau@redhat.com wrote: > 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. Would it be simpler to just state that ALL commands that want to use optional arguments must use a boxed type? Then the function signature is simple (a single boxed type), and the logic of the optional parameter is now contained within the type rather than within the function signature.
On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote: > > However, I think it would be simpler, and better, if we piped the > > generated code to clang-format (when available). I made a simple patch > > for that too. > > Piping through indent or clang-format may well give us neater results > for less effort. > > We might want to dumb down generator code then. Indeed, this approach seems like it might be worth pursuing (our generator doesn't have to worry about spacing, because we do that in a second pass with something that will still produce human-legible final results). > >> > 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? > >> > >> Yes: throw an error. Assertions are for programming errors. This isn't > >> a programming error, it's a limitation of the current implementation. > >> > >> How hard would it be to lift the limitation? > > > > Taking this as a problematic example: > > > > void function(first, > > #ifdef A > > a, > > #endif > > #ifdef B > > b > > #endif > > ) I am NOT a fan of preprocessor conditionals mid-function-signature. It gets really nasty, really fast. Is there any way we can have: struct S { #ifdef A type a; #endif #ifdef B type b; #endif }; void function(struct S) so that the preprocessor conditionals never appear inside ()?
Hi On Tue, Feb 28, 2023 at 7:55 PM Eric Blake <eblake@redhat.com> wrote: > On Tue, Feb 07, 2023 at 06:25:32PM +0400, marcandre.lureau@redhat.com > wrote: > > 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. > > Would it be simpler to just state that ALL commands that want to use > optional arguments must use a boxed type? Then the function signature > is simple (a single boxed type), and the logic of the optional > parameter is now contained within the type rather than within the > function signature. > > Possibly, but I would prefer if we leave that as a FIXME, because review rate isn't quite fast, and it delays other patches that are pending for months already ...
On Tue, Feb 28, 2023 at 09:58:01AM -0600, Eric Blake wrote: > On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote: > > > However, I think it would be simpler, and better, if we piped the > > > generated code to clang-format (when available). I made a simple patch > > > for that too. > > > > Piping through indent or clang-format may well give us neater results > > for less effort. > > > > We might want to dumb down generator code then. > > Indeed, this approach seems like it might be worth pursuing (our > generator doesn't have to worry about spacing, because we do that in a > second pass with something that will still produce human-legible final > results). > > > >> > 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? > > >> > > >> Yes: throw an error. Assertions are for programming errors. This isn't > > >> a programming error, it's a limitation of the current implementation. > > >> > > >> How hard would it be to lift the limitation? > > > > > > Taking this as a problematic example: > > > > > > void function(first, > > > #ifdef A > > > a, > > > #endif > > > #ifdef B > > > b > > > #endif > > > ) > > I am NOT a fan of preprocessor conditionals mid-function-signature. > It gets really nasty, really fast. Is there any way we can have: > > struct S { > #ifdef A > type a; > #endif > #ifdef B > type b; > #endif > }; > > void function(struct S) > > so that the preprocessor conditionals never appear inside ()? I'd question whether we should be doing conditional arguments at all. IMHO having an API contract that changes based on configuration file settings is going to be nothing but trouble. Not only does it make the declaration ugly, but all callers become ugly too with conditionals. It will lead to bugs where a caller is written and tested with one build combination, and find it forgot the conditional calling needed for a different build combination. Any fields that we conditionally disable must already be marked as optional in the schema, to indicate to mgmt apps that they may or may not be present depend on what QEMU build the app is talking to. So if they're optional, what is wrong with generating the arguments unconditionally and just leaving them unused/unset in builds that don't require them ? I think it'd be fine if the qmp_getfd API decl in QEMU had an 'const char *wsainfo' field even on Linux builds. The Linux impl can simply ignore it, or raise an error if it is set. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 28, 2023 at 09:58:01AM -0600, Eric Blake wrote: >> On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote: >> > > However, I think it would be simpler, and better, if we piped the >> > > generated code to clang-format (when available). I made a simple patch >> > > for that too. >> > >> > Piping through indent or clang-format may well give us neater results >> > for less effort. >> > >> > We might want to dumb down generator code then. >> >> Indeed, this approach seems like it might be worth pursuing (our >> generator doesn't have to worry about spacing, because we do that in a >> second pass with something that will still produce human-legible final >> results). >> >> > >> > 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? >> > >> >> > >> Yes: throw an error. Assertions are for programming errors. This isn't >> > >> a programming error, it's a limitation of the current implementation. >> > >> >> > >> How hard would it be to lift the limitation? >> > > >> > > Taking this as a problematic example: >> > > >> > > void function(first, >> > > #ifdef A >> > > a, >> > > #endif >> > > #ifdef B >> > > b >> > > #endif >> > > ) >> >> I am NOT a fan of preprocessor conditionals mid-function-signature. >> It gets really nasty, really fast. Is there any way we can have: >> >> struct S { >> #ifdef A >> type a; >> #endif >> #ifdef B >> type b; >> #endif >> }; >> >> void function(struct S) >> >> so that the preprocessor conditionals never appear inside ()? Yes, there is a way: we have conditional object type members, and we have 'boxed': true for commands. > I'd question whether we should be doing conditional arguments > at all. Note that conditional arguments already exist. This patch merely makes them work as advertized even without 'boxed': true. If we decide they shouldn't exist, we need to make the generator reject them. But what *exactly* shouldn't exist? In my view, a command takes *one* argument (of complex type) and returns *one* result (of any type, but only because we had to grandfather non-complex types). For *struct* arguments, we support a special way to pass arguments: unboxed. Actually used for most commands. Support for conditional members of complex types implies support for conditional members of (complex) argument types. We could restrict them to boxed. Perhaps we should. > IMHO having an API contract that changes based on configuration > file settings is going to be nothing but trouble. Not only does > it make the declaration ugly, but all callers become ugly too > with conditionals. It will lead to bugs where a caller is written > and tested with one build combination, and find it forgot the > conditional calling needed for a different build combination. Two separate points: ugly and test matrix. "Ugly" is a valid point. Ugly in two generated places (generated declaration and handwritten definition) is kind of bad, ugly in all places (plus all callers) is worse. "Test matrix" applies to conditionals anywhere, not just command arguments. You could perhaps argue certain conditionals are more problematic than others. > Any fields that we conditionally disable must already be marked > as optional in the schema, to indicate to mgmt apps that they > may or may not be present depend on what QEMU build the app is > talking to. Not quite. Neither the code nor docs/devel/qapi-code-gen.txt require conditional members of complex types to be optional. qapi-schema-test.json covers conditional mandatory members: { 'struct': 'TestIfStruct', 'data': { 'foo': 'int', ---> 'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_BAR'} }, 'if': 'TEST_IF_STRUCT' } "Optional" isn't for indicating "that a member may or may not be present depend on what QEMU build" via introspection, it's for indicating the member exists in *this* QEMU build, but it need not be present on the wire, no more, no less. We could perhaps derive "conditional member must be optional" from our compatibility promise, but my cold-addled brain is not up to that task right now. > So if they're optional, what is wrong with generating the arguments > unconditionally and just leaving them unused/unset in builds that > don't require them ? I think it'd be fine if the qmp_getfd API > decl in QEMU had an 'const char *wsainfo' field even on Linux > builds. The Linux impl can simply ignore it, or raise an error if > it is set. Raise an error. Silently ignoring is no good. Taking a step back... both proposed solutions are kind of ugly. Let me recap: 1. QMP command getfd takes an additional argument when QEMU runs on a Windows host. a. The additional argument is mandatory. b. The additional argument in optional, but only nominally; if absent, the command fails. 2. QMP comamnd getfd takes an optional argument. If it's present and QEMU runs on a POSIX host, the command fails. Likewise when it's absent and QEMU runs on a Windows host. What about 3. have an additional command conditional on CONFIG_WIN32? Existing getfd stays the same: always fails when QEMU runs on a Windows host. The new command exists only when QEMU runs on a Windows host.
Hi On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote: > What about 3. have an additional command conditional on CONFIG_WIN32? > Existing getfd stays the same: always fails when QEMU runs on a Windows > host. The new command exists only when QEMU runs on a Windows host. This is what was suggested initially: https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/ I also like it better, as a specific command for windows sockets, less ways to use it wrongly.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote: >> What about 3. have an additional command conditional on CONFIG_WIN32? >> Existing getfd stays the same: always fails when QEMU runs on a Windows >> host. The new command exists only when QEMU runs on a Windows host. We could additionally deprecate getfd for Windows. > This is what was suggested initially: > https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/ > > I also like it better, as a specific command for windows sockets, less > ways to use it wrongly. Daniel, what do you think?
On Thu, Mar 02, 2023 at 07:58:28AM +0100, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi > > > > On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote: > >> What about 3. have an additional command conditional on CONFIG_WIN32? > >> Existing getfd stays the same: always fails when QEMU runs on a Windows > >> host. The new command exists only when QEMU runs on a Windows host. > > We could additionally deprecate getfd for Windows. > > > This is what was suggested initially: > > https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/ > > > > I also like it better, as a specific command for windows sockets, less > > ways to use it wrongly. > > Daniel, what do you think? I wasn't especially a fan of platform specific APIs, but perhaps in retrospect it will be the lesser of two evils. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Mar 02, 2023 at 07:58:28AM +0100, Markus Armbruster wrote: >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >> > Hi >> > >> > On Wed, Mar 1, 2023 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> What about 3. have an additional command conditional on CONFIG_WIN32? >> >> Existing getfd stays the same: always fails when QEMU runs on a Windows >> >> host. The new command exists only when QEMU runs on a Windows host. >> >> We could additionally deprecate getfd for Windows. >> >> > This is what was suggested initially: >> > https://patchew.org/QEMU/20230103110814.3726795-1-marcandre.lureau@redhat.com/20230103110814.3726795-9-marcandre.lureau@redhat.com/ >> > >> > I also like it better, as a specific command for windows sockets, less >> > ways to use it wrongly. >> >> Daniel, what do you think? > > I wasn't especially a fan of platform specific APIs, but perhaps in > retrospect it will be the lesser of two evils. Marc-André, let's go to your initial approach then. This breaks the dependence between this patch and the remainder of the series. We still need to consider this patch anyway. It partially fills gaps in code generation for unboxed conditional arguments. Partially, because the last argument must not be conditional. It needs work to either lift this restriction, or reject such arguments properly. Either way, the generated code will be ugly, and the handwritten code interfacing with it will be ugly, too. I'd like to tighten the restriction instead: conditional arguments require boxed. Less ugly and less awkward to document, I think.
Markus Armbruster <armbru@redhat.com> writes: [...] > I'd like to tighten the restriction instead: conditional arguments > require boxed. Less ugly and less awkward to document, I think. I had a look, and realized that doing it right will be a bit more involved than I thought, since a couple of test cases need fixing. I'll give it a try.
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: 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() + if memb.ifcond.is_present(): + ret += '\n' + memb.ifcond.gen_endif() if extra: - ret += sep + extra + ret += extra return ret if ret else 'void' 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: 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': {},