Re: [Qemu-devel] [PATCH v2 17/54] qapi: add 'if' condition on entity objects

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

Hi

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > Take 'if' from expression, and use it to construct entity objects.
> > Shared implicit objects must share the same 'if' condition.
> 
> Shared by what?

Shared by various make_implicit_object_type() users.

> 
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/qapi.py | 81
> >  +++++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 53 insertions(+), 28 deletions(-)
> >
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index a94d93ada4..dc40d74abb 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -992,7 +992,7 @@ def check_exprs(exprs):
> >  #
> >  
> >  class QAPISchemaEntity(object):
> > -    def __init__(self, name, info, doc):
> > +    def __init__(self, name, info, doc, ifcond=None):
> >          assert isinstance(name, str)
> >          self.name = name
> >          # For explicitly defined entities, info points to the (explicit)
> 
> Not visible here: QAPISchemaType inherits this.
> 
> > @@ -1002,6 +1002,7 @@ class QAPISchemaEntity(object):
> >          # such place).
> >          self.info = info
> >          self.doc = doc
> > +        self.ifcond = ifcond
> >  
> >      def c_name(self):
> >          return c_name(self.name)
> > @@ -1118,8 +1119,8 @@ class QAPISchemaBuiltinType(QAPISchemaType):
> >  
> >  
> >  class QAPISchemaEnumType(QAPISchemaType):
> > -    def __init__(self, name, info, doc, values, prefix):
> > -        QAPISchemaType.__init__(self, name, info, doc)
> > +    def __init__(self, name, info, doc, values, prefix, ifcond=None):
> > +        QAPISchemaType.__init__(self, name, info, doc, ifcond)
> >          for v in values:
> >              assert isinstance(v, QAPISchemaMember)
> >              v.set_owner(name)
> > @@ -1162,6 +1163,7 @@ class QAPISchemaArrayType(QAPISchemaType):
> >      def check(self, schema):
> >          self.element_type = schema.lookup_type(self._element_type_name)
> >          assert self.element_type
> > +        self.ifcond = self.element_type.ifcond
> 
> This is subtler than it looks on first glance.
> 
> All the other entities set self.ifcond in their constructor to the true
> value passed in as argument.
> 
> QAPISchemaArrayType doesn't take such an argument.  Instead, it inherits
> its .ifcond from its .element_type.  However, .element_type isn't known
> at construction time if it's a forward reference.  We therefore delay
> setting it until .check() time.  You do the same for .ifcond (no
> choice).
> 
> Before .check(), .ifcond is None, because the constructor sets it that
> way: it calls QAPISchemaType.__init__() without passing a ifcond
> argument, which then sets self.ifcond to its default argument None.
> 
> Pitfall: accessing ent.ifcond before ent.check() works *except* when ent
> is an array type.  Hmm.
> 
> >  
> >      def is_implicit(self):
> >          return True
> > @@ -1183,11 +1185,12 @@ class QAPISchemaArrayType(QAPISchemaType):
> >  
> >  
> >  class QAPISchemaObjectType(QAPISchemaType):
> > -    def __init__(self, name, info, doc, base, local_members, variants):
> > +    def __init__(self, name, info, doc, base, local_members, variants,
> > +                 ifcond=None):
> >          # struct has local_members, optional base, and no variants
> >          # flat union has base, variants, and no local_members
> >          # simple union has local_members, variants, and no base
> > -        QAPISchemaType.__init__(self, name, info, doc)
> > +        QAPISchemaType.__init__(self, name, info, doc, ifcond)
> >          assert base is None or isinstance(base, str)
> >          for m in local_members:
> >              assert isinstance(m, QAPISchemaObjectTypeMember)
> > @@ -1375,8 +1378,8 @@ class
> > QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> >  
> >  
> >  class QAPISchemaAlternateType(QAPISchemaType):
> > -    def __init__(self, name, info, doc, variants):
> > -        QAPISchemaType.__init__(self, name, info, doc)
> > +    def __init__(self, name, info, doc, variants, ifcond):
> > +        QAPISchemaType.__init__(self, name, info, doc, ifcond)
> >          assert isinstance(variants, QAPISchemaObjectTypeVariants)
> >          assert variants.tag_member
> >          variants.set_owner(name)
> > @@ -1413,8 +1416,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
> >  
> >  class QAPISchemaCommand(QAPISchemaEntity):
> >      def __init__(self, name, info, doc, arg_type, ret_type,
> > -                 gen, success_response, boxed):
> > -        QAPISchemaEntity.__init__(self, name, info, doc)
> > +                 gen, success_response, boxed, ifcond):
> > +        QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
> >          assert not arg_type or isinstance(arg_type, str)
> >          assert not ret_type or isinstance(ret_type, str)
> >          self._arg_type_name = arg_type
> > @@ -1451,8 +1454,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
> >  
> >  
> >  class QAPISchemaEvent(QAPISchemaEntity):
> > -    def __init__(self, name, info, doc, arg_type, boxed):
> > -        QAPISchemaEntity.__init__(self, name, info, doc)
> > +    def __init__(self, name, info, doc, arg_type, boxed, ifcond):
> > +        QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
> >          assert not arg_type or isinstance(arg_type, str)
> >          self._arg_type_name = arg_type
> >          self.arg_type = None
> 
> Hmm.  You're adding parameter ifcond to all entity constructors.  The
> entity constructors' signatures all look like this:
> 
>     def __init__(<common parameters>, <specific parameters>)
> 
> where <common parameters> is self, name, info, doc.
> 
> Since ifcond is common, let's add it to <common parameters>.  Pick a
> position you like.
> 

ok

> > @@ -1547,11 +1550,11 @@ class QAPISchema(object):
> >      def _make_enum_members(self, values):
> >          return [QAPISchemaMember(v) for v in values]
> >  
> > -    def _make_implicit_enum_type(self, name, info, values):
> > +    def _make_implicit_enum_type(self, name, info, values, ifcond):
> >          # See also QAPISchemaObjectTypeMember._pretty_owner()
> >          name = name + 'Kind'   # Use namespace reserved by add_name()
> >          self._def_entity(QAPISchemaEnumType(
> > -            name, info, None, self._make_enum_members(values), None))
> > +            name, info, None, self._make_enum_members(values), None,
> > ifcond))
> >          return name
> 
> Called by _def_union_type() to create the implicit tag enum, with the
> same ifcond it passes to QAPISchemaObjectType().  Thus, the implicit
> type's ifcond matches the ifcond of its only user.  Good.
> 
> >  
> >      def _make_array_type(self, element_type, info):
> > @@ -1560,22 +1563,29 @@ class QAPISchema(object):
> >              self._def_entity(QAPISchemaArrayType(name, info,
> >              element_type))
> >          return name
> >  
> > -    def _make_implicit_object_type(self, name, info, doc, role, members):
> > +    def _make_implicit_object_type(self, name, info, doc, role, members,
> > +                                   ifcond=None):
> >          if not members:
> >              return None
> >          # See also QAPISchemaObjectTypeMember._pretty_owner()
> >          name = 'q_obj_%s-%s' % (name, role)
> > -        if not self.lookup_entity(name, QAPISchemaObjectType):
> > +        typ = self.lookup_entity(name, QAPISchemaObjectType)
> > +        if typ:
> > +            # FIXME: generating the disjunction of all conditions
> 
> What are "all conditions"?  I hope I can shed some light on that below.
> 
> > +            assert ifcond == typ.ifcond
> > +        else:
> >              self._def_entity(QAPISchemaObjectType(name, info, doc, None,
> > -                                                  members, None))
> > +                                                  members, None, ifcond))
> >          return name
> 
> Several callers:
> 
> * _make_simple_variant() to create a wrapper type, with the wrapped
>   type's ifcond.  The wrapped type's ifcond is necessary for the wrapper
>   type to compile.  It's not sufficient for it to be needed.  The
>   "needed" condition is the disjunction of all users' ifcond.
> 
>   In other words, your code generates condition that is correct, but not
>   tight.  Your FIXME is about tightening it.  I'd make it a TODO,
>   because nothing is actually broken now.
> 
>   Need a comment explaining this.  Perhaps:
> 
>         if typ:
>             # The implicit object type has multiple users.  This can
>             # happen only for simple unions' implicit wrapper types.
>             # Its ifcond should be the disjunction of its user's
>             # ifconds.  Not implemented.  Instead, we always pass the
>             # wrapped type's ifcond, which is trivially the same for all
>             # users.  It's also necessary for the wrapper to compile.
>             # But it's not tight: the disjunction need not imply it.  We
>             # may end up compiling useless wrapper types.
>             # TODO kill simple unions or implement the disjunction
> 
>   I hate simple unions.

ok

> 
> * _def_union_type() to create an implicit base type, with same ifcond it
>   passes to QAPISchemaObjectType().  Thus, the implicit base type's
>   ifcond matches the ifcond of its only user.  Good.
> 
> * _def_command() to create an implicit argument type, with the same
>   ifcond it passes to QAPISchemaCommand().  Thus, the implicit argument
>   type's ifcond matches the ifcond of its only user.  Good.
> 
> * _def_event() likewise.
> 
> I still can't make sense of the commit message's "Shared implicit
> objects must share the same 'if' condition."
> 
> >  
> >      def _def_enum_type(self, expr, info, doc):
> >          name = expr['enum']
> >          data = expr['data']
> >          prefix = expr.get('prefix')
> > +        ifcond = expr.get('if')
> >          self._def_entity(QAPISchemaEnumType(
> > -            name, info, doc, self._make_enum_members(data), prefix))
> > +            name, info, doc, self._make_enum_members(data), prefix,
> > +            ifcond))
> >  
> >      def _make_member(self, name, typ, info):
> >          optional = False
> > @@ -1595,9 +1605,10 @@ class QAPISchema(object):
> >          name = expr['struct']
> >          base = expr.get('base')
> >          data = expr['data']
> > +        ifcond = expr.get('if')
> >          self._def_entity(QAPISchemaObjectType(name, info, doc, base,
> >                                                self._make_members(data,
> >                                                info),
> > -                                              None))
> > +                                              None, ifcond))
> >  
> >      def _make_variant(self, case, typ):
> >          return QAPISchemaObjectTypeVariant(case, typ)
> > @@ -1606,19 +1617,23 @@ class QAPISchema(object):
> >          if isinstance(typ, list):
> >              assert len(typ) == 1
> >              typ = self._make_array_type(typ[0], info)
> > +        type_entity = self.lookup_type(typ)
> >          typ = self._make_implicit_object_type(
> > -            typ, info, None, 'wrapper', [self._make_member('data', typ,
> > info)])
> > +            typ, info, None, 'wrapper',
> > +            [self._make_member('data', typ, info)], type_entity.ifcond)
> >          return QAPISchemaObjectTypeVariant(case, typ)
> 
> This is where you create the wrapper type with the wrapped type's
> ifcond.
> 
> I don't like the name type_entity.  I'd simply eliminate the variable.
> 

ok

> >  
> >      def _def_union_type(self, expr, info, doc):
> >          name = expr['union']
> >          data = expr['data']
> >          base = expr.get('base')
> > +        ifcond = expr.get('if')
> >          tag_name = expr.get('discriminator')
> >          tag_member = None
> >          if isinstance(base, dict):
> > -            base = (self._make_implicit_object_type(
> > -                name, info, doc, 'base', self._make_members(base, info)))
> > +            base = self._make_implicit_object_type(
> > +                name, info, doc, 'base', self._make_members(base, info),
> > +                ifcond)
> >          if tag_name:
> >              variants = [self._make_variant(key, value)
> >                          for (key, value) in data.iteritems()]
> 
> This is where you create a union's implicit base type with the union's
> ifcond.
> 
> > @@ -1627,18 +1642,21 @@ class QAPISchema(object):
> >              variants = [self._make_simple_variant(key, value, info)
> >                          for (key, value) in data.iteritems()]
> >              typ = self._make_implicit_enum_type(name, info,
> > -                                                [v.name for v in
> > variants])
> > +                                                [v.name for v in
> > variants],
> > +                                                ifcond)
> >              tag_member = QAPISchemaObjectTypeMember('type', typ, False)
> >              members = [tag_member]
> 
> This is where you create a union's implicit tag enum with the union's ifcond.
> 
> >          self._def_entity(
> >              QAPISchemaObjectType(name, info, doc, base, members,
> >                                   QAPISchemaObjectTypeVariants(tag_name,
> >                                                                tag_member,
> > -                                                              variants)))
> > +                                                              variants),
> > +                                 ifcond))
> >  
> >      def _def_alternate_type(self, expr, info, doc):
> >          name = expr['alternate']
> >          data = expr['data']
> > +        ifcond = expr.get('if')
> >          variants = [self._make_variant(key, value)
> >                      for (key, value) in data.iteritems()]
> >          tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
> > @@ -1646,7 +1664,8 @@ class QAPISchema(object):
> >              QAPISchemaAlternateType(name, info, doc,
> >                                      QAPISchemaObjectTypeVariants(None,
> >                                                                   tag_member,
> > -
> > variants)))
> > +
> > variants),
> > +                                    ifcond))
> >  
> >      def _def_command(self, expr, info, doc):
> >          name = expr['command']
> > @@ -1655,23 +1674,29 @@ class QAPISchema(object):
> >          gen = expr.get('gen', True)
> >          success_response = expr.get('success-response', True)
> >          boxed = expr.get('boxed', False)
> > +        ifcond = expr.get('if')
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > -                name, info, doc, 'arg', self._make_members(data, info))
> > +                name, info, doc, 'arg', self._make_members(data, info),
> > +                ifcond)
> 
> This is where you create a command's implicit argument type with the
> command's ifcond.
> 
> >          if isinstance(rets, list):
> >              assert len(rets) == 1
> >              rets = self._make_array_type(rets[0], info)
> >          self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
> > -                                           gen, success_response, boxed))
> > +                                           gen, success_response, boxed,
> > +                                           ifcond))
> >  
> >      def _def_event(self, expr, info, doc):
> >          name = expr['event']
> >          data = expr.get('data')
> >          boxed = expr.get('boxed', False)
> > +        ifcond = expr.get('if')
> >          if isinstance(data, OrderedDict):
> >              data = self._make_implicit_object_type(
> > -                name, info, doc, 'arg', self._make_members(data, info))
> > -        self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed))
> > +                name, info, doc, 'arg', self._make_members(data, info),
> > +                ifcond)
> 
> This is where you create an event's implicit argument type with the
> event's ifcond.
> 
> > +        self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed,
> > +                                         ifcond))
> >  
> >      def _def_exprs(self):
> >          for expr_elem in self.exprs:
>