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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 07/07/2016 04:52 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>>>
>
>>> @@ -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.

Yes.

I like to avoid .check() recursion whenever practical, to keep the
termination argument simple, but since I can't see how to avoid it here,
we'll go with your code.

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

You're right.

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

Let's restore the assertions, because they do double-duty documenting
intent here.

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

Yes, but it should be few and simple changes, thus quick to review.