On Tue, Feb 20, 2024 at 8:29 AM Markus Armbruster <arm...@redhat.com> wrote: > > John Snow <js...@redhat.com> writes: > > > Instead of using the None value for the members field, use a dedicated > > "checking" value to detect recursive misconfigurations. > > > > This is intended to assist with subsequent patches which will seek to > > remove the "None" value from the members field (which can never be set > > legally after the final call to check()) in order to simplify static > > typing of that field, by avoiding needing assertions littered at many > > callsites. > > > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > scripts/qapi/schema.py | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index d4d3c3bbcee..a459016e148 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -458,19 +458,21 @@ def __init__(self, name, info, doc, ifcond, features, > > self.local_members = local_members > > self.variants = variants > > self.members = None > > + self._checking = False > > > > def check(self, schema): > > # 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: > > + if self._checking: > > # Recursed: C struct contains itself > > raise QAPISemError(self.info, > > "object %s contains itself" % self.name) > > + if self._checked: > > + # A previous .check() completed: nothing to do > > + return > > The diff would be easier to read if you could keep the order... You > don't due to the subtle change of the state predicates. More on that > below. > > > > > + self._checking = True > > super().check(schema) > > assert self._checked and self.members is None > > > > @@ -495,7 +497,8 @@ def check(self, schema): > > self.variants.check(schema, seen) > > self.variants.check_clash(self.info, seen) > > > > - self.members = members # mark completed > > + self.members = members > > + self._checking = False # 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 > > We .check() entities on after the other. *Except* > QAPISchemaObjectType.check() "calls another type T's .check() exactly > when the C struct emitted by gen_object() contains that T's C struct". > If the recursion loops, the schema asks for C structs containing > themselves. To catch this, we have QAPISchemaType objects go through > three states: > > 1. Not yet checked. > > 2. Being checked; object.check() is on the call stack. > > 3. Checked, i.e. object.check() completed. > > How to recognize the states before the patch: > > 1. not ._checked and .members is None > > 2. ._checked and .members is None > > 3. ._checked and .members is not None > > Since .members is not None implies .checked, we simply use > .members is not None. > > We go from state 1. to 2. in super().check(). > > We go from state 2. to 3. at # mark completed. > > Note that .checked becomes true well before we finish checking. This is > admittedly confusing. If you can think of a less confusing name, ...
"checking", of course ;) I won't change it here, but... that's what I'd be drawn to ... > > The patch's aim is to avoid use of .members, to enable the next patch. > > I don't doubt that your solution works, but trying to understand how it > works makes my tired brain go owww! > > State invariants (I think): > > 1. not ._checked and .members is None and not ._checking > > 2. ._checked and .members is None and ._checking > > 3. ._checked and .members is not None and not ._checking > > We can then recognize states without use of .members: > > 1. not ._checked and not ._checking > > Since not ._checked implies not .checking, we can simply use > not ._checked. > > 2. ._checked and ._checking > > A deliciously confusing predicate, isn't it? > > 3. ._checked and not ._checking > > Deep breath... alright, here's the stupidest replacement for use of > .members that could possibly work: add a flag, ensure it's True exactly > when .members is not None. Like this: OK, sure. As long as the next patch also shakes out just fine... > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index d4d3c3bbce..095831baf2 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -458,12 +458,13 @@ def __init__(self, name, info, doc, ifcond, features, > self.local_members = local_members > self.variants = variants > self.members = None > + self._check_complete = False > > def check(self, schema): > # 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: > + if self._check_complete: > # A previous .check() completed: nothing to do > return > if self._checked: > @@ -495,7 +496,8 @@ def check(self, schema): > self.variants.check(schema, seen) > self.variants.check_clash(self.info, seen) > > - self.members = members # mark completed > + self.members = members > + self._check_complete = True # 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 > > > Thoughts? >