Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Eric Blake]

On 11/10/2015 02:15 AM, Markus Armbruster wrote:

>> On the other hand, we've been arguing that check() should populate
>> everything after construction prior to anything else being run; and not
>> running Variant.type.check() during Variants.check() of flat unions
>> feels like we may have a hole (a flat union will have to inline its
>> types to the overall JSON object, and inlining types requires access to
>> type.members - but as written, we aren't populating them until
>> Variants.check_clash()).  I can play with hoisting the type.check() out
>> of type.check_clash() and instead keep base.check() in type.check(), and
>> add variant.type.check() in Variants.check() (but only for unions, not
>> for alternates), if you are interested.
> 
> My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" adds
> QAPISchemaObjectTypeMember.check_clash() without changing the common
> protocol.  The new QAPISchemaObjectTypeMember.check_clash() is merely a
> helper for QAPISchemaObjectType.check().
> 
> The two .check_clash() you add (one in this patch, one in the previous
> one) are different: both contain calls of QAPISchemaObjectType.check().
> 
> I feel the .check() calls are too important to be buried deep like that.
> I'd stick to prior practice and put the .check() calls right into
> .check().  Obviously, the .check_clash() methods may only called after
> .check() then, but that's nothing new.
> 
> Fixup for your previous patch:
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4c56935..357127d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>              vseen = dict(seen)
>              assert isinstance(v.type, QAPISchemaObjectType)
>              assert not v.type.variants       # not implemented
> -            v.type.check(schema)
>              for m in v.type.members:
>                  m.check_clash(vseen)
>  
> @@ -1077,6 +1076,7 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def check(self, schema, tag_type):
>          QAPISchemaObjectTypeMember.check(self, schema)
>          assert self.name in tag_type.values
> +        self.type.check(schema)
>  

Won't quite work.  You are right that we must call
self.type.check(schema) for variants used by a union; but calling it for
ALL variants used by an alternate is wrong, because self.type for at
least one branch of an alternate will not be an instance of
QAPISchemaObjectType.  However, I'm currently testing whether it is safe
to check to just blindly check an object branch of an alternate, if
present (and that should not lead to cycles, since alternates have no
base class and since we don't allow one alternate type as a variant of
another alternate), in which case the fixup for 23/30 is more like:

diff --git i/scripts/qapi.py w/scripts/qapi.py
index a005c87..25fa642 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
             vseen = dict(seen)
             assert isinstance(v.type, QAPISchemaObjectType)
             assert not v.type.variants       # not implemented
-            v.type.check(schema)
             for m in v.type.members:
                 m.check_clash(vseen)

@@ -1077,6 +1076,8 @@ class
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def check(self, schema, tag_type):
         QAPISchemaObjectTypeMember.check(self, schema)
         assert self.name in tag_type.values
+        if isinstance(self.type, QAPISchemaObjectType):
+            self.type.check(schema)

     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
@@ -1098,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType):

     def check(self, schema):
         self.variants.tag_member.check(schema)
+        # Not calling self.variants.check_clash(), because there's
+        # nothing to clash with
         self.variants.check(schema, {})

     def json_type(self):




-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature