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