Re: [Qemu-devel] [PATCH v2 32/54] qapi: add 'if' to struct members

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  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.