Message ID | 1447224690-9743-29-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > It should be fairly obvious that qapi base classes need to > form an acyclic graph, since QMP cannot specify the same > key more than once, while base classes are included as flat > members alongside other members added by the child. But the > old check_member_clash() parser function was not prepared to > check for this, and entered an infinite recursion (at least > until python gives up, complaining about nesting too deep). Nitpick: Python. > > Now that check_member_clash() has been recently removed, > attempts at self-inheritance trigger an assertion failure > introduced by commit ac88219a. The obvious fix is to turn > the assertion into a conditional. Exactly. Like all the assertions in check() methods, it was always meant as a placeholder for a semantic check. > This patch includes both the test and the fix, since the .err > file output for the unfixed case is not useful (particularly > when it was warning about unbounded recursion, as that limit > may be platform-specific). > > We don't need to worry about cycles in flat unions (neither > the base nor a variant class can be a union) (neither the base type nor the type of a variant ...) Note that if they could, this check would report the cycle just fine. > nor in alternates > (alternate branches cannot themselves be an alternate). If they could, all it took to detect the cycle would be a v.type.check() in QAPISchemaAlternateType.check(). > And > even the case of using the same class for a flat union base > class and one its variants is already caught by the fact that > both uses will introduce the same member name (or will be okay > if the class is empty). That's not a cycle, that's a diamond, isn't it? > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v11 (no v10): rename base-cycle to base-cycle-indirect, and add > base-cycle-direct; touch up commit message > v9: no change > v8: improve commit message > v7: improve commit message > v6: rebase to earlier info changes > --- > scripts/qapi.py | 6 +++++- > tests/Makefile | 2 ++ > tests/qapi-schema/base-cycle-direct.err | 1 + > tests/qapi-schema/base-cycle-direct.exit | 1 + > tests/qapi-schema/base-cycle-direct.json | 2 ++ > tests/qapi-schema/base-cycle-direct.out | 0 > tests/qapi-schema/base-cycle-indirect.err | 1 + > tests/qapi-schema/base-cycle-indirect.exit | 1 + > tests/qapi-schema/base-cycle-indirect.json | 3 +++ > tests/qapi-schema/base-cycle-indirect.out | 0 > 10 files changed, 16 insertions(+), 1 deletion(-) > create mode 100644 tests/qapi-schema/base-cycle-direct.err > create mode 100644 tests/qapi-schema/base-cycle-direct.exit > create mode 100644 tests/qapi-schema/base-cycle-direct.json > create mode 100644 tests/qapi-schema/base-cycle-direct.out > create mode 100644 tests/qapi-schema/base-cycle-indirect.err > create mode 100644 tests/qapi-schema/base-cycle-indirect.exit > create mode 100644 tests/qapi-schema/base-cycle-indirect.json > create mode 100644 tests/qapi-schema/base-cycle-indirect.out > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 08a366e..e60c1d8 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -941,7 +941,11 @@ class QAPISchemaObjectType(QAPISchemaType): > self.members = None > > def check(self, schema): > - assert self.members is not False # not running in cycles > + if self.members is False: # check for cycles > + assert self._base_name > + raise QAPIExprError(self.info, > + "Object %s cyclically depends on %s" > + % (self.name, self._base_name)) Well, it cyclically depends on *itself*. If the cycle is of length one, this message becomes Object Foo cyclically depends on Foo Sounds awkward. If the cycle is longer, it becomes Object Foo cyclically depends on Bar where Foo -> Bar happens to be the beginning of the cycle. Borders on misleading. We could print the complete cycle, but that feels like too much work for too little gain. Let's dumb down the message to something like "Type Foo contains itself"[*]. > if self.members: > return > self.members = False # mark as being checked > diff --git a/tests/Makefile b/tests/Makefile > index cdff7a4..2706126 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -253,6 +253,8 @@ qapi-schema += bad-ident.json > qapi-schema += bad-type-bool.json > qapi-schema += bad-type-dict.json > qapi-schema += bad-type-int.json > +qapi-schema += base-cycle-direct.json > +qapi-schema += base-cycle-indirect.json > qapi-schema += command-int.json > qapi-schema += comments.json > qapi-schema += double-data.json > diff --git a/tests/qapi-schema/base-cycle-direct.err b/tests/qapi-schema/base-cycle-direct.err > new file mode 100644 > index 0000000..46fc436 > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-direct.err > @@ -0,0 +1 @@ > +tests/qapi-schema/base-cycle-direct.json:2: Object Foo cyclically depends on Foo > diff --git a/tests/qapi-schema/base-cycle-direct.exit b/tests/qapi-schema/base-cycle-direct.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-direct.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/base-cycle-direct.json b/tests/qapi-schema/base-cycle-direct.json > new file mode 100644 > index 0000000..dff6fab > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-direct.json > @@ -0,0 +1,2 @@ > +# we reject a loop in base classes > +{ 'struct': 'Foo', 'base': 'Foo', 'data': {} } I gather you didn't like 'Loopy'. Pity... > diff --git a/tests/qapi-schema/base-cycle-direct.out b/tests/qapi-schema/base-cycle-direct.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/base-cycle-indirect.err b/tests/qapi-schema/base-cycle-indirect.err > new file mode 100644 > index 0000000..9a1ca44 > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-indirect.err > @@ -0,0 +1 @@ > +tests/qapi-schema/base-cycle-indirect.json:2: Object Base1 cyclically depends on Base2 > diff --git a/tests/qapi-schema/base-cycle-indirect.exit b/tests/qapi-schema/base-cycle-indirect.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-indirect.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/base-cycle-indirect.json b/tests/qapi-schema/base-cycle-indirect.json > new file mode 100644 > index 0000000..2866772 > --- /dev/null > +++ b/tests/qapi-schema/base-cycle-indirect.json > @@ -0,0 +1,3 @@ > +# we reject a loop in base classes > +{ 'struct': 'Base1', 'base': 'Base2', 'data': {} } > +{ 'struct': 'Base2', 'base': 'Base1', 'data': {} } > diff --git a/tests/qapi-schema/base-cycle-indirect.out b/tests/qapi-schema/base-cycle-indirect.out > new file mode 100644 > index 0000000..e69de29 [*] I'm tempted to make it "Foo is a type, not a Klein bottle", but I acknowledge our users may not share my sense of humor ;)
diff --git a/scripts/qapi.py b/scripts/qapi.py index 08a366e..e60c1d8 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -941,7 +941,11 @@ class QAPISchemaObjectType(QAPISchemaType): self.members = None def check(self, schema): - assert self.members is not False # not running in cycles + if self.members is False: # check for cycles + assert self._base_name + raise QAPIExprError(self.info, + "Object %s cyclically depends on %s" + % (self.name, self._base_name)) if self.members: return self.members = False # mark as being checked diff --git a/tests/Makefile b/tests/Makefile index cdff7a4..2706126 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -253,6 +253,8 @@ qapi-schema += bad-ident.json qapi-schema += bad-type-bool.json qapi-schema += bad-type-dict.json qapi-schema += bad-type-int.json +qapi-schema += base-cycle-direct.json +qapi-schema += base-cycle-indirect.json qapi-schema += command-int.json qapi-schema += comments.json qapi-schema += double-data.json diff --git a/tests/qapi-schema/base-cycle-direct.err b/tests/qapi-schema/base-cycle-direct.err new file mode 100644 index 0000000..46fc436 --- /dev/null +++ b/tests/qapi-schema/base-cycle-direct.err @@ -0,0 +1 @@ +tests/qapi-schema/base-cycle-direct.json:2: Object Foo cyclically depends on Foo diff --git a/tests/qapi-schema/base-cycle-direct.exit b/tests/qapi-schema/base-cycle-direct.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/base-cycle-direct.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/base-cycle-direct.json b/tests/qapi-schema/base-cycle-direct.json new file mode 100644 index 0000000..dff6fab --- /dev/null +++ b/tests/qapi-schema/base-cycle-direct.json @@ -0,0 +1,2 @@ +# we reject a loop in base classes +{ 'struct': 'Foo', 'base': 'Foo', 'data': {} } diff --git a/tests/qapi-schema/base-cycle-direct.out b/tests/qapi-schema/base-cycle-direct.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/base-cycle-indirect.err b/tests/qapi-schema/base-cycle-indirect.err new file mode 100644 index 0000000..9a1ca44 --- /dev/null +++ b/tests/qapi-schema/base-cycle-indirect.err @@ -0,0 +1 @@ +tests/qapi-schema/base-cycle-indirect.json:2: Object Base1 cyclically depends on Base2 diff --git a/tests/qapi-schema/base-cycle-indirect.exit b/tests/qapi-schema/base-cycle-indirect.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/base-cycle-indirect.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/base-cycle-indirect.json b/tests/qapi-schema/base-cycle-indirect.json new file mode 100644 index 0000000..2866772 --- /dev/null +++ b/tests/qapi-schema/base-cycle-indirect.json @@ -0,0 +1,3 @@ +# we reject a loop in base classes +{ 'struct': 'Base1', 'base': 'Base2', 'data': {} } +{ 'struct': 'Base2', 'base': 'Base1', 'data': {} }
It should be fairly obvious that qapi base classes need to form an acyclic graph, since QMP cannot specify the same key more than once, while base classes are included as flat members alongside other members added by the child. But the old check_member_clash() parser function was not prepared to check for this, and entered an infinite recursion (at least until python gives up, complaining about nesting too deep). Now that check_member_clash() has been recently removed, attempts at self-inheritance trigger an assertion failure introduced by commit ac88219a. The obvious fix is to turn the assertion into a conditional. This patch includes both the test and the fix, since the .err file output for the unfixed case is not useful (particularly when it was warning about unbounded recursion, as that limit may be platform-specific). We don't need to worry about cycles in flat unions (neither the base nor a variant class can be a union) nor in alternates (alternate branches cannot themselves be an alternate). And even the case of using the same class for a flat union base class and one its variants is already caught by the fact that both uses will introduce the same member name (or will be okay if the class is empty). Signed-off-by: Eric Blake <eblake@redhat.com> --- v11 (no v10): rename base-cycle to base-cycle-indirect, and add base-cycle-direct; touch up commit message v9: no change v8: improve commit message v7: improve commit message v6: rebase to earlier info changes --- scripts/qapi.py | 6 +++++- tests/Makefile | 2 ++ tests/qapi-schema/base-cycle-direct.err | 1 + tests/qapi-schema/base-cycle-direct.exit | 1 + tests/qapi-schema/base-cycle-direct.json | 2 ++ tests/qapi-schema/base-cycle-direct.out | 0 tests/qapi-schema/base-cycle-indirect.err | 1 + tests/qapi-schema/base-cycle-indirect.exit | 1 + tests/qapi-schema/base-cycle-indirect.json | 3 +++ tests/qapi-schema/base-cycle-indirect.out | 0 10 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 tests/qapi-schema/base-cycle-direct.err create mode 100644 tests/qapi-schema/base-cycle-direct.exit create mode 100644 tests/qapi-schema/base-cycle-direct.json create mode 100644 tests/qapi-schema/base-cycle-direct.out create mode 100644 tests/qapi-schema/base-cycle-indirect.err create mode 100644 tests/qapi-schema/base-cycle-indirect.exit create mode 100644 tests/qapi-schema/base-cycle-indirect.json create mode 100644 tests/qapi-schema/base-cycle-indirect.out diff --git a/tests/qapi-schema/base-cycle-indirect.out b/tests/qapi-schema/base-cycle-indirect.out new file mode 100644 index 0000000..e69de29