From: Markus Armbruster <armbru@redhat.com>
To: qemu-devel@nongnu.org
Cc: marcandre.lureau@redhat.com, mdroth@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH 18/19] qapi: Fix excessive QAPISchemaEntity.check() recursion
Date: Sat, 14 Sep 2019 17:35:05 +0200 [thread overview]
Message-ID: <20190914153506.2151-19-armbru@redhat.com> (raw)
In-Reply-To: <20190914153506.2151-1-armbru@redhat.com>
Entity checking goes back to commit ac88219a6c "qapi: New QAPISchema
intermediate representation", v2.5.0. It's designed to work as
follows: QAPISchema.check() calls .check() for all the schema's
entities. An entity's .check() recurses into another entity's
.check() only if the C struct generated for the former contains the C
struct generated for the latter (pointers don't count). This is used
to detect "object contains itself".
There are two instances of this:
* An object's C struct contains its base's C struct
QAPISchemaObjectType.check() calls self.base.check()
* An object's C struct contains its variants' C structs
QAPISchemaObjectTypeVariants().check calls v.type.check(). Since
commit b807a1e1e3 "qapi: Check for QAPI collisions involving variant
members", v2.6.0.
Thus, only object types can participate in recursion.
QAPISchemaObjectType.check() is made for that: it checks @self when
called the first time, recursing into base and variants, it reports an
"contains itself" error when this recursion reaches an object being
checked, and does nothing it reaches an object that has been checked
already.
The other .check() may safely assume they get called exactly once.
Sadly, this design has since eroded:
* QAPISchemaCommand.check() and QAPISchemaEvent.check() call
.args_type.check(). Since commit c818408e44 "qapi: Implement boxed
types for commands/events", v2.7.0. Harmless, since args_type can
only be an object type.
* QAPISchemaEntity.check() calls ._ifcond.check() when inheriting the
condition from another type. Since commit 4fca21c1b0 qapi: leave
the ifcond attribute undefined until check(), v3.0.0. This makes
simple union wrapper types recurse into the wrapped type (nothing
else uses this condition inheritance). The .check() of types used
as simple union branch type get called multiple times.
* QAPISchemaObjectType.check() calls its super type's .check()
*before* the conditional handling multiple calls. Also since commit
4fca21c1b0. QAPISchemaObjectType.check()'s guard against multiple
checking doesn't protect QAPISchemaEntity.check().
* QAPISchemaArrayType.check() calls .element_type.check(). Also since
commit 4fca21c1b0. The .check() of types used as array element
types get called multiple times.
Commit 56a4689582 "qapi: Fix array first used in a different module"
(v4.0.0) added more code relying on this .element_type.check().
The absence of explosions suggests the .check() involved happen to be
effectively idempotent.
Fix the unwanted recursion anyway:
* QAPISchemaCommand.check() and QAPISchemaEvent.check() calling
.args_type.check() is unnecessary. Delete the calls.
* Fix QAPISchemaObjectType.check() to call its super type's .check()
after the conditional handling multiple calls.
* A QAPISchemaEntity's .ifcond becomes valid at .check(). This is due
to arrays and simple unions.
Most types get ifcond and info passed to their constructor.
Array types don't: they get it from their element type, which
becomes known only in .element_type.check().
The implicit wrapper object types for simple union branches don't:
they get it from the wrapped type, which might be an array.
Ditch the idea to set .ifcond in .check(). Instead, turn it into a
property and compute it on demand. Safe because it's only used
after the schema has been checked.
Most types simply return the ifcond passed to their constructor.
Array types forward to their .element_type instead, and the wrapper
types forward to the wrapped type.
* A QAPISchemaEntity's .module becomes valid at .check(). This is
because computing it needs info and schema.fname, and because array
types get it from their element type instead.
Make it a property just like .ifcond.
Additionally, have QAPISchemaEntity.check() assert it gets called at
most once, so the design invariant will stick this time. Neglecting
that was entirely my fault.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/common.py | 74 +++++++++++++++++++++++++++++-------------
1 file changed, 52 insertions(+), 22 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index e2c87d1349..c199a2b58c 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -1155,7 +1155,7 @@ class QAPISchemaEntity(object):
def __init__(self, name, info, doc, ifcond=None):
assert name is None or isinstance(name, str)
self.name = name
- self.module = None
+ self._module = None
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
# For implicitly defined entities, info points to a place that
@@ -1164,21 +1164,27 @@ class QAPISchemaEntity(object):
self.info = info
self.doc = doc
self._ifcond = ifcond or []
+ self._checked = False
def c_name(self):
return c_name(self.name)
def check(self, schema):
- if isinstance(self._ifcond, QAPISchemaType):
- # inherit the condition from a type
- typ = self._ifcond
- typ.check(schema)
- self.ifcond = typ.ifcond
- else:
- self.ifcond = self._ifcond
+ assert not self._checked
if self.info:
- self.module = os.path.relpath(self.info['file'],
- os.path.dirname(schema.fname))
+ self._module = os.path.relpath(self.info['file'],
+ os.path.dirname(schema.fname))
+ self._checked = True
+
+ @property
+ def ifcond(self):
+ assert self._checked
+ return self._ifcond
+
+ @property
+ def module(self):
+ assert self._checked
+ return self._module
def is_implicit(self):
return not self.info
@@ -1353,9 +1359,16 @@ class QAPISchemaArrayType(QAPISchemaType):
QAPISchemaType.check(self, schema)
self.element_type = schema.lookup_type(self._element_type_name)
assert self.element_type
- self.element_type.check(schema)
- self.module = self.element_type.module
- self.ifcond = self.element_type.ifcond
+
+ @property
+ def ifcond(self):
+ assert self._checked
+ return self.element_type.ifcond
+
+ @property
+ def module(self):
+ assert self._checked
+ return self.element_type.module
def is_implicit(self):
return True
@@ -1402,13 +1415,20 @@ class QAPISchemaObjectType(QAPISchemaType):
self.features = features
def check(self, schema):
- QAPISchemaType.check(self, schema)
- if self.members is False: # check for cycles
+ # This calls another type T's .check() exactly when the C
+ # struct emitted by gen_object() contains that T's C struct
+ # (pointers don't count).
+ if self.members is not None:
+ # A previous .check() completed: nothing to do
+ return
+ if self._checked:
+ # Recursed: C struct contains itself
raise QAPISemError(self.info,
"Object %s contains itself" % self.name)
- if self.members is not None: # already checked
- return
- self.members = False # mark as being checked
+
+ QAPISchemaType.check(self, schema)
+ assert self._checked and self.members is None
+
seen = OrderedDict()
if self._base_name:
self.base = schema.lookup_type(self._base_name)
@@ -1420,10 +1440,11 @@ class QAPISchemaObjectType(QAPISchemaType):
m.check_clash(self.info, seen)
if self.doc:
self.doc.connect_member(m)
- self.members = seen.values()
+ members = seen.values()
+
if self.variants:
self.variants.check(schema, seen)
- assert self.variants.tag_member in self.members
+ assert self.variants.tag_member in members
self.variants.check_clash(self.info, seen)
# Features are in a name space separate from members
@@ -1434,6 +1455,8 @@ class QAPISchemaObjectType(QAPISchemaType):
if self.doc:
self.doc.check()
+ self.members = members # mark completed
+
# Check that the members of this type do not cause duplicate JSON members,
# and update seen to track the members seen so far. Report any errors
# on behalf of info, which is not necessarily self.info
@@ -1442,6 +1465,15 @@ class QAPISchemaObjectType(QAPISchemaType):
for m in self.members:
m.check_clash(info, seen)
+ @property
+ def ifcond(self):
+ assert self._checked
+ if isinstance(self._ifcond, QAPISchemaType):
+ # Simple union wrapper type inherits from wrapped type;
+ # see _make_implicit_object_type()
+ return self._ifcond.ifcond
+ return self._ifcond
+
def is_implicit(self):
# See QAPISchema._make_implicit_object_type(), as well as
# _def_predefineds()
@@ -1658,7 +1690,6 @@ class QAPISchemaCommand(QAPISchemaEntity):
if self._arg_type_name:
self.arg_type = schema.lookup_type(self._arg_type_name)
assert isinstance(self.arg_type, QAPISchemaObjectType)
- self.arg_type.check(schema)
assert not self.arg_type.variants or self.boxed
elif self.boxed:
raise QAPISemError(self.info, "Use of 'boxed' requires 'data'")
@@ -1687,7 +1718,6 @@ class QAPISchemaEvent(QAPISchemaEntity):
if self._arg_type_name:
self.arg_type = schema.lookup_type(self._arg_type_name)
assert isinstance(self.arg_type, QAPISchemaObjectType)
- self.arg_type.check(schema)
assert not self.arg_type.variants or self.boxed
elif self.boxed:
raise QAPISemError(self.info, "Use of 'boxed' requires 'data'")
--
2.21.0
next prev parent reply other threads:[~2019-09-14 15:43 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-14 15:34 [Qemu-devel] [PATCH 00/19] qapi: Frontend fixes and cleanups Markus Armbruster
2019-09-14 15:34 ` [Qemu-devel] [PATCH 01/19] tests/qapi-schema: Cover unknown pragma Markus Armbruster
2019-09-17 16:36 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 02/19] tests/qapi-schema: Delete two redundant tests Markus Armbruster
2019-09-17 16:57 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 03/19] tests/qapi-schema: Demonstrate misleading optional tag error Markus Armbruster
2019-09-17 17:36 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 04/19] tests/qapi-schema: Demonstrate broken discriminator errors Markus Armbruster
2019-09-17 17:43 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking Markus Armbruster
2019-09-17 17:47 ` Eric Blake
2019-09-23 11:55 ` Markus Armbruster
2019-09-23 12:55 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 06/19] tests/qapi-schema: Demonstrate suboptimal lexical errors Markus Armbruster
2019-09-17 17:49 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 07/19] qapi: Use quotes more consistently in frontend error messages Markus Armbruster
2019-09-17 17:52 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 08/19] qapi: Improve reporting of lexical errors Markus Armbruster
2019-09-17 17:54 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 09/19] qapi: Remove null from schema language Markus Armbruster
2019-09-17 18:01 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 10/19] qapi: Fix broken discriminator error messages Markus Armbruster
2019-09-17 18:05 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 11/19] qapi: Reject blank 'if' conditions in addition to empty ones Markus Armbruster
2019-09-17 18:06 ` Eric Blake
2019-09-14 15:34 ` [Qemu-devel] [PATCH 12/19] qapi: Fix missing 'if' checks in struct, union, alternate 'data' Markus Armbruster
2019-09-17 18:10 ` Eric Blake
2019-09-14 15:35 ` [Qemu-devel] [PATCH 13/19] qapi: Normalize 'if' in check_exprs(), like other sugar Markus Armbruster
2019-09-17 18:14 ` Eric Blake
2019-09-23 11:59 ` Markus Armbruster
2019-09-14 15:35 ` [Qemu-devel] [PATCH 14/19] qapi: Simplify check_keys() Markus Armbruster
2019-09-17 19:01 ` Eric Blake
2019-09-14 15:35 ` [Qemu-devel] [PATCH 15/19] qapi: Clean up around check_known_keys() Markus Armbruster
2019-09-17 19:03 ` Eric Blake
2019-09-14 15:35 ` [Qemu-devel] [PATCH 16/19] qapi: Delete useless check_exprs() code for simple union kind Markus Armbruster
2019-09-17 19:05 ` Eric Blake
2019-09-14 15:35 ` [Qemu-devel] [PATCH 17/19] qapi: Fix to .check() empty structs just once Markus Armbruster
2019-09-17 19:06 ` Eric Blake
2019-09-14 15:35 ` Markus Armbruster [this message]
2019-09-17 19:26 ` [Qemu-devel] [PATCH 18/19] qapi: Fix excessive QAPISchemaEntity.check() recursion Eric Blake
2019-09-23 12:01 ` Markus Armbruster
2019-09-14 15:35 ` [Qemu-devel] [PATCH 19/19] qapi: Assert .visit() and .check_clash() run only after .check() Markus Armbruster
2019-09-17 19:27 ` Eric Blake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190914153506.2151-19-armbru@redhat.com \
--to=armbru@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).