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?
> @@ -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'.
> '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)
> self._print_variants(variants)
> self._print_if(ifcond)
Need negative tests, but I guess they're next.
