Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into QAPISchemaEntity objects

[Markus Armbruster]

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

> Built-in objects remain unconditional.  Explicitly defined objects
> use the condition specified in the schema.  Implicitly defined
> objects inherit their condition from their users.  For most of them,
> there is exactly one user, so the condition to use is obvious.  The
> exception is the wrapper types generated for simple union variants,
> which can be shared by any number of simple unions.  The tight
> condition would be the disjunction of the conditions of these simple
> unions.  For now, use wrapped type's condition instead.  Much

the wrapped type's

> simpler and good enough for now.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi.py | 89 
> ++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 57 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 20c1abf915..0f55caa18d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1000,7 +1000,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)
> @@ -1010,6 +1010,7 @@ class QAPISchemaEntity(object):
>          # such place).
>          self.info = info
>          self.doc = doc
> +        self.ifcond = ifcond
>  
>      def c_name(self):
>          return c_name(self.name)
> @@ -1126,8 +1127,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, ifcond, values, prefix):
> +        QAPISchemaType.__init__(self, name, info, doc, ifcond)
>          for v in values:
>              assert isinstance(v, QAPISchemaMember)
>              v.set_owner(name)
> @@ -1162,7 +1163,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>  
>  class QAPISchemaArrayType(QAPISchemaType):
>      def __init__(self, name, info, element_type):
> -        QAPISchemaType.__init__(self, name, info, None)
> +        QAPISchemaType.__init__(self, name, info, None, None)
>          assert isinstance(element_type, str)
>          self._element_type_name = element_type
>          self.element_type = None
> @@ -1170,6 +1171,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
>  
>      def is_implicit(self):
>          return True

In my review of v2, I wrote:

    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.

The problem is of course more general.  We commonly initialize
attributes to None in .init(), then set their real value in .check().
Accessing the attribute before .check() yields None.  If we're lucky,
the code that accesses the attribute prematurely chokes on None.

It won't for .ifcond, because None is a legitimate value.

Perhaps we should leave such attributes undefined until .check().  What
do you think?  No need to tie that idea to this series, though.

[...]

With the commit message tidied up:
Reviewed-by: Markus Armbruster <address@hidden>