From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47435) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLPry-0001Zp-PQ for qemu-devel@nongnu.org; Fri, 08 Jul 2016 03:06:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLPrv-00075F-H3 for qemu-devel@nongnu.org; Fri, 08 Jul 2016 03:06:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60973) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLPrv-00075A-8b for qemu-devel@nongnu.org; Fri, 08 Jul 2016 03:06:35 -0400 From: Markus Armbruster References: <1467514729-29366-1-git-send-email-eblake@redhat.com> <1467514729-29366-9-git-send-email-eblake@redhat.com> <87vb0hpvoj.fsf@dusky.pond.sub.org> <577E7CFF.4000408@redhat.com> Date: Fri, 08 Jul 2016 09:06:32 +0200 In-Reply-To: <577E7CFF.4000408@redhat.com> (Eric Blake's message of "Thu, 7 Jul 2016 10:02:07 -0600") Message-ID: <87furkd2x3.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > On 07/07/2016 04:52 AM, Markus Armbruster wrote: >> Eric Blake 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 >>> > >>> @@ -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.