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

[Markus Armbruster <armbru@redhat.com>]

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, ...

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:

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?