Hi
----- Original Message -----
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > scripts/qapi.py | 18 ++++++++++++++----
> > tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
> > tests/qapi-schema/qapi-schema-test.out | 4 +++-
> > tests/qapi-schema/test-qapi.py | 3 ++-
> > 4 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index 201e425842..946df83854 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -678,7 +678,13 @@ def check_type(info, source, value, allow_array=False,
> > return
> >
> > if not allow_dict:
> > - raise QAPISemError(info, "%s should be a type name" % source)
> > + if isinstance(value, dict) and 'type' in value:
>
> "not allow_dict" has become a lie:
>
> Not your patch's fault: we sometimes check for dict, and sometimes for
> OrderedDict. If there's a reason, it's not obvious. If there isn't, we
> should be more consistent.
>
> > + check_type(info, source, value['type'], allow_array,
> > + allow_dict, allow_optional, allow_metas)
> > + check_if(value, info)
>
> Where are dictionary keys other than 'type' and 'if' rejected?
>
> > + return
> > + else:
> > + raise QAPISemError(info, "%s should be a type name" % source)
> >
> > if not isinstance(value, OrderedDict):
> > raise QAPISemError(info,
>
> check_type() is used all over the place. Your commit message claims
> that your patch only affects struct members. That's not obvious, so
> down I go the next rabbit hole you present me :)
>
> First: no change unless allow_dict. Let's review the callers that pass
> allow_dict=True.
>
> * check_command() for arguments when not boxed
>
> * check_event() for arguments when not boxed
>
> * check_union() for a flat union's base
>
> * check_struct() for the struct's members
>
> As far as I can tell, these all accept {'type': TYPE-NAME, 'if': 'COND'}
> in addition to TYPE-NAME as values in their dictionaries. That's not
> what the commit message claims. Which one needs fixing, commit message
> or code?
>
> I guess it's the commit message, because we want to support 'if' in all
> these places, don't we?
Yes
>
> > @@ -1332,8 +1338,8 @@ class QAPISchemaMember(object):
> >
> >
> > class QAPISchemaObjectTypeMember(QAPISchemaMember):
> > - def __init__(self, name, typ, optional):
> > - QAPISchemaMember.__init__(self, name)
> > + def __init__(self, name, typ, optional, ifcond=None):
> > + QAPISchemaMember.__init__(self, name, ifcond)
> > assert isinstance(typ, str)
> > assert isinstance(optional, bool)
> > self._type_name = typ
> > @@ -1612,13 +1618,17 @@ class QAPISchema(object):
> >
> > def _make_member(self, name, typ, info):
> > optional = False
> > + ifcond = None
> > if name.startswith('*'):
> > name = name[1:]
> > optional = True
> > + if isinstance(typ, dict):
> > + ifcond = typ.get('if')
> > + typ = typ['type']
> > if isinstance(typ, list):
> > assert len(typ) == 1
> > typ = self._make_array_type(typ[0], info)
> > - return QAPISchemaObjectTypeMember(name, typ, optional)
> > + return QAPISchemaObjectTypeMember(name, typ, optional, ifcond)
> >
> > def _make_members(self, data, info):
> > return [self._make_member(key, value, info)
>
> We call _make_members() for exactly the four things I enumerated above:
> command arguments, event arguments, flat union's base, struct members.
> Good.
>
> > diff --git a/tests/qapi-schema/qapi-schema-test.json
> > b/tests/qapi-schema/qapi-schema-test.json
> > index ad2b405d83..b136e4855f 100644
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -191,7 +191,8 @@
> >
> > # test 'if' condition handling
> >
> > -{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> > +{ 'struct': 'TestIfStruct', 'data':
> > + { 'foo': 'int', 'bar': { 'type': 'int', 'if':
> > 'defined(TEST_IF_STRUCT_BAR)'} },
>
> Long line, suggest to break it before 'bar'.
>
ok
> > 'if': 'defined(TEST_IF_STRUCT)' }
> >
> > { 'enum': 'TestIfEnum', 'data':
> > @@ -204,8 +205,10 @@
> > { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar':
> > 'TestStruct' },
> > 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
> >
> > -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar':
> > 'TestIfEnum' },
> > +{ 'command': 'TestIfCmd', 'data':
> > + { 'foo': 'TestIfStruct', 'bar': { 'type': 'TestIfEnum', 'if':
> > 'defined(TEST_IF_CMD_BAR)' } },
>
> Likewise.
>
> > 'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }
> >
> > -{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
> > +{ 'event': 'TestIfEvent', 'data':
> > + { 'foo': 'TestIfStruct', 'bar': { 'type': 'TestIfEnum', 'if':
> > 'defined(TEST_IF_EVT_BAR)' } },
>
> Likewise.
>
> > 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
> > diff --git a/tests/qapi-schema/qapi-schema-test.out
> > b/tests/qapi-schema/qapi-schema-test.out
> > index 211c367632..5d2f18e4aa 100644
> > --- a/tests/qapi-schema/qapi-schema-test.out
> > +++ b/tests/qapi-schema/qapi-schema-test.out
> > @@ -67,6 +67,7 @@ event TestIfEvent q_obj_TestIfEvent-arg
> > if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
> > object TestIfStruct
> > member foo: int optional=False
> > + member bar: int optional=False if=defined(TEST_IF_STRUCT_BAR)
> > if defined(TEST_IF_STRUCT)
> > object TestIfUnion
> > member type: TestIfUnionKind optional=False
> > @@ -197,10 +198,11 @@ object q_obj_EVENT_D-arg
> > member enum3: EnumOne optional=True
> > object q_obj_TestIfCmd-arg
> > member foo: TestIfStruct optional=False
> > - member bar: TestIfEnum optional=False
> > + member bar: TestIfEnum optional=False if=defined(TEST_IF_CMD_BAR)
> > if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
> > object q_obj_TestIfEvent-arg
> > member foo: TestIfStruct optional=False
> > + member bar: TestIfEnum optional=False if=defined(TEST_IF_EVT_BAR)
> > if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
> > object q_obj_TestStruct-wrapper
> > member data: TestStruct optional=False
> > diff --git a/tests/qapi-schema/test-qapi.py
> > b/tests/qapi-schema/test-qapi.py
> > index 70054848f0..5d2f67a1d3 100644
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -31,7 +31,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> > print ' base %s' % base.name
> > for m in members:
> > print ' member %s: %s optional=%s' % \
> > - (m.name, m.type.name, m.optional)
> > + (m.name, m.type.name, m.optional) + \
> > + (' if=%s' % m.ifcond if m.ifcond else '')
>
> Let's keep it simple & stupid:
>
> print ' member %s: %s optional=%s if=%s' % \
> (m.name, m.type.name, m.optional, m.ifcond)
>
I would rather not modify every existing test please.
The last version of the series uses the trailing , trick instead:
print '%s optional=%s' % (m.type.name, m.optional),
if m.ifcond:
print 'if=%s' % m.ifcond,
print
I think this is easier to deal with, is that ok for you too?
> > self._print_variants(variants)
> > self._print_if(ifcond)
>
> Need negative tests, but I guess they're next.
>
