[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 05/49] qapi: leave the ifcond attribute undef
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 05/49] qapi: leave the ifcond attribute undefined until check() |
Date: |
Tue, 19 Jun 2018 11:06:48 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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?
> ---
> 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?
>
> 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.
> else:
> 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.
- Re: [Qemu-devel] [PATCH v3 05/49] qapi: leave the ifcond attribute undefined until check(),
Markus Armbruster <=