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

Commit Message

John Snow Feb. 1, 2024, 10:42 p.m. UTC

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 <[e-mail]>
---
 scripts/qapi/schema.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Markus Armbruster Feb. 20, 2024, 1:29 p.m. UTC | #1

John Snow <[e-mail]> 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 <[e-mail]>
> ---
>  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?

John Snow March 12, 2024, 9:04 p.m. UTC | #2

On Tue, Feb 20, 2024 at 8:29 AM Markus Armbruster <[e-mail]> wrote:
>
> John Snow <[e-mail]> 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 <[e-mail]>
> > ---
> >  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?
>

Patch

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
 
+        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