[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for comman
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events |
Date: |
Fri, 08 Jul 2016 09:06:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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.
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.
[Qemu-devel] [PATCH v8 07/16] qapi: Plumb in 'box' to qapi generator lower levels, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 09/16] block: Simplify block_set_io_throttle, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 16/16] schema: Drop pointless empty type CpuInfoOther, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 12/16] qapi: Change Netdev into a flat union, Eric Blake, 2016/07/02
Re: [Qemu-devel] [PATCH for-2.7 v8 00/16] qapi netdev_add introspection (post-introspection cleanups subset F), Markus Armbruster, 2016/07/07