Re: [Qemu-devel] [PATCH v3 05/49] qapi: leave the ifcond attribute undefined until check()

[Marc-André Lureau]

Hi

On Tue, Jun 19, 2018 at 11:06 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> 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.
>>
>> Leave the ifcond attribute undefined until check().
>
> Drawback: pylint complains.  We'll live.
>
>>
>> Suggested-by: Markus Armbruster <address@hidden>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> Reviewed-by: Markus Armbruster <address@hidden>
>
> Shouldn't this be squashed into the previous patch?

I would rather keep it seperate, as it makes reviewing both a bit
easier to me. But feel free to squash on commit.

>
>> ---
>>  scripts/qapi/common.py | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index d8ab3d8f7f..eb07d641ab 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -1026,13 +1026,19 @@ class QAPISchemaEntity(object):
>>          # such place).
>>          self.info = info
>>          self.doc = doc
>> -        self.ifcond = listify_cond(ifcond)
>> +        self._ifcond = ifcond  # self.ifcond is set only after .check()
>>
>>      def c_name(self):
>>          return c_name(self.name)
>>
>>      def check(self, schema):
>> -        pass
>> +        if isinstance(self._ifcond, QAPISchemaType):
>> +            # inherit the condition from a type
>> +            typ = self._ifcond
>> +            typ.check(schema)
>> +            self.ifcond = typ.ifcond
>> +        else:
>> +            self.ifcond = listify_cond(self._ifcond)
>
> Whenever we add a .check(), we need to prove QAPISchema.check()'s
> recursion still terminates, and terminates the right way.
>
> Argument before this patch: we recurse only into types contained in
> types, e.g. an object type's base type, and we detect and report cycles
> as "Object %s contains itself", in QAPISchemaObjectType.check().
>
> The .check() added here recurses into a type.  If this creates a cycle,
> it'll be caught and reported as "contains itself".  We still need to
> show that the error message remains accurate.
>
> We .check() here to inherit .ifcond from a type.  As far as I can tell,
> we use this inheritance feature only to inherit an array's condition
> from its element type.  That's safe, because an array does contain its
> elements.
>
> This is hardly a rigorous proof.  Just enough to make me believe your
> code works.
>
> However, I suspect adding the inheritance feature at the entity level
> complicates the correctness argument without real need.  Can we restrict
> it to array elements?  Have QAPISchemaArrayType.check() resolve
> type-valued ._ifcond, and all the others choke on it?

There is also implicit object types.

>
>>
>>      def is_implicit(self):
>>          return not self.info
>> @@ -1169,6 +1175,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>>          self.prefix = prefix
>>
>>      def check(self, schema):
>> +        QAPISchemaType.check(self, schema)
>>          seen = {}
>>          for v in self.values:
>>              v.check_clash(self.info, seen)
>> @@ -1201,8 +1208,10 @@ class QAPISchemaArrayType(QAPISchemaType):
>>          self.element_type = None
>>
>>      def check(self, schema):
>> +        QAPISchemaType.check(self, schema)
>>          self.element_type = schema.lookup_type(self._element_type_name)
>>          assert self.element_type
>> +        self.element_type.check(schema)
>>          self.ifcond = self.element_type.ifcond
>>
>>      def is_implicit(self):
>> @@ -1245,6 +1254,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>          self.members = None
>>
>>      def check(self, schema):
>> +        QAPISchemaType.check(self, schema)
>>          if self.members is False:               # check for cycles
>>              raise QAPISemError(self.info,
>>                                 "Object %s contains itself" % self.name)
>> @@ -1427,6 +1437,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>>          self.variants = variants
>>
>>      def check(self, schema):
>> +        QAPISchemaType.check(self, schema)
>>          self.variants.tag_member.check(schema)
>>          # Not calling self.variants.check_clash(), because there's nothing
>>          # to clash with
>> @@ -1470,6 +1481,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>>          self.allow_oob = allow_oob
>>
>>      def check(self, schema):
>> +        QAPISchemaEntity.check(self, schema)
>>          if self._arg_type_name:
>>              self.arg_type = schema.lookup_type(self._arg_type_name)
>>              assert (isinstance(self.arg_type, QAPISchemaObjectType) or
>> @@ -1504,6 +1516,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
>>          self.boxed = boxed
>>
>>      def check(self, schema):
>> +        QAPISchemaEntity.check(self, schema)
>>          if self._arg_type_name:
>>              self.arg_type = schema.lookup_type(self._arg_type_name)
>>              assert (isinstance(self.arg_type, QAPISchemaObjectType) or
>> @@ -1633,7 +1646,7 @@ class QAPISchema(object):
>>              # 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
>> -            assert ifcond == typ.ifcond
>> +            assert ifcond == typ._ifcond
>
> pylint complains
>
>     W:1649,29: Access to a protected member _ifcond of a client class 
> (protected-access)
>
> Layering violation.  Tolerable, I think.
>

yeah, alternative would be to add an assert_ifcond() method in type..?
I'll add a # pylint: disable=protected-access for now

>>              self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
>>                                                    None, members, None))
>> @@ -1679,7 +1692,7 @@ class QAPISchema(object):
>>              assert len(typ) == 1
>>              typ = self._make_array_type(typ[0], info)
>>          typ = self._make_implicit_object_type(
>> -            typ, info, None, self.lookup_type(typ).ifcond,
>> +            typ, info, None, self.lookup_type(typ),
>>              'wrapper', [self._make_member('data', typ, info)])
>>          return QAPISchemaObjectTypeVariant(case, typ)
>
> Perhaps other attributes that become valid only at .check() time should
> receive the same treatment.  Not necessarily in this series, not
> necessarily by you.  A TODO comment wouldn't hurt, though.
>

It doesn't look obvious to me which should receive the same
treatement. I'll leave that to you to figure out :)


-- 
Marc-André Lureau