Re: [PATCH v3 13/20] qapi/schema: split "checked" field into "checking" and "checked"

[John Snow]

On Tue, Feb 20, 2024 at 8:29 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@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 <jsnow@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?
>