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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@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.
>