Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events

[Eric Blake]

On 07/07/2016 04:52 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Turn on the ability to pass command and event arguments in
>> a single boxed parameter, which must name a non-empty type
>> (although the type can be a struct with all optional members).
>> For structs, it makes it possible to pass a single qapi type
>> instead of a breakout of all struct members (useful if the
>> arguments are already in a struct or if the number of members
>> is large); for other complex types, it is now possible to use
>> a union or alternate as the data for a command or event.
>>
>> The empty type may be technically feasible if needed down the
>> road, but it's easier to forbid it now and relax things to allow
>> it later, than it is to allow it now and have to special case
>> how the generated 'q_empty' type is handled (see commit 7ce106a9
>> for reasons why nothing is generated for the empty type).  An
>> alternate type is never considered empty.
>>
>> Generated code is unchanged, as long as no client uses the
>> new feature.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>> @@ -1180,9 +1195,18 @@ class QAPISchemaCommand(QAPISchemaEntity):
>>      def check(self, schema):
>>          if self._arg_type_name:
>>              self.arg_type = schema.lookup_type(self._arg_type_name)
>> -            assert isinstance(self.arg_type, QAPISchemaObjectType)
>> -            assert not self.arg_type.variants   # not implemented
>> -            assert not self.box                 # not implemented
>> +            assert (isinstance(self.arg_type, QAPISchemaObjectType) or
>> +                    isinstance(self.arg_type, QAPISchemaAlternateType))
>> +            self.arg_type.check(schema)
> 
> qapi.py recurses .check() only when necessary, because undisciplined
> recursion can easily become cyclic.
> 
> I think you need self.arg_type.check() here so you can
> self.arg_type.is_empty() below.  Correct?

Correct.  And should not be unbounded - a command depends on a type, but
no type depends on a command, so this does not introduce new recursion.

> 
>> +            if self.box:
>> +                if self.arg_type.is_empty():
>> +                    raise QAPIExprError(self.info,
>> +                                        "Cannot use 'box' with empty type")
>> +            else:
>> +                assert not self.arg_type.variants
> 
> Lost: assert isinstance(self.arg_type, QAPISchemaObjectType), or the
> equivalent assert not isinstance(self.arg_type, QAPISchemaAlternateType).

Or rather, implicitly hidden.  Only QAPISchemaObjectType and
QAPISchemaAlternateType have a .is_empty() or .variants, so if any other
type is passed, python will complain about a missing attribute (which is
just as effective as the assert() used to be).

> 
>> +        elif self.box:
>> +            raise QAPIExprError(self.info,
>> +                                "Use of 'box' requires 'data'")
>>          if self._ret_type_name:
>>              self.ret_type = schema.lookup_type(self._ret_type_name)
>>              assert isinstance(self.ret_type, QAPISchemaType)
>> @@ -1204,9 +1228,18 @@ class QAPISchemaEvent(QAPISchemaEntity):
>>      def check(self, schema):
>>          if self._arg_type_name:
>>              self.arg_type = schema.lookup_type(self._arg_type_name)
>> -            assert isinstance(self.arg_type, QAPISchemaObjectType)
>> -            assert not self.arg_type.variants   # not implemented
>> -            assert not self.box                 # not implemented
>> +            assert (isinstance(self.arg_type, QAPISchemaObjectType) or
>> +                    isinstance(self.arg_type, QAPISchemaAlternateType))
>> +            self.arg_type.check(schema)
>> +            if self.box:
>> +                if self.arg_type.is_empty():
>> +                    raise QAPIExprError(self.info,
>> +                                        "Cannot use 'box' with empty type")
>> +            else:
>> +                assert not self.arg_type.variants
> 
> Likewise.

And same argument about being implicitly correct.  I can either document
this in the commit message (to call it out as intentional) or restore
the asserts; up to you which is cleaner.


>>  == Client JSON Protocol introspection ==
>>
> [Tests snipped, they look good...]
> 
> Having read PATCH 07+08 another time, I got one more stylistic remark:
> I'd name the flag @boxed, not @box.  It's not a box, it's a flag that
> tells us that whatever it applies to is boxed.

Sounds reasonable, so looks like a v9 is worth posting.

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

signature.asc
Description: OpenPGP digital signature