From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Y1-0006OM-Ch for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZK8Xu-0000g2-N6 for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51845) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Xu-0000eU-Gu for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:06 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-43-git-send-email-armbru@redhat.com> <55B171D0.1060509@redhat.com> Date: Tue, 28 Jul 2015 14:04:36 +0200 In-Reply-To: <55B171D0.1060509@redhat.com> (Eric Blake's message of "Thu, 23 Jul 2015 16:59:28 -0600") Message-ID: <871tfswkrv.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 42/47] qapi-schema: Fix up misleading specification of netdev_add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake writes: > On 07/01/2015 02:22 PM, Markus Armbruster wrote: >> It doesn't take a 'props' argument, let alone one in the format >> "NAME=VALUE,..." >> >> The bogus arguments specification doesn't matter due to 'gen': false. >> Clean it up to be incomplete rather than wrong, and document the >> incompleteness. >> >> Signed-off-by: Markus Armbruster >> --- >> docs/qapi-code-gen.txt | 2 +- >> qapi-schema.json | 13 +++++++------ >> 2 files changed, 8 insertions(+), 7 deletions(-) >> > >> +++ b/qapi-schema.json >> @@ -2062,11 +2062,12 @@ >> # >> # @id: the name of the new network backend >> # >> -# @props: #optional a list of properties to be passed to the backend in >> -# the format 'name=value', like 'ifname=tap0,script=no' >> +# Additional arguments depend on the type. > > In other words, this would be a perfect candidate to write as taking a > flat union as the argument, where 'type' is the discriminator, and where > the additional arguments are branches of the union. Except that we > don't yet support passing a union as the direct "arguments":{...} of a > QMP call, so we still have some design work to do if we want to go > there. Later, of course. Exactly. I started hacking in that direction, but had to stop to get this stuff out. >> # >> -# Notes: The semantics of @props is not well defined. Future >> commands will be >> -# introduced that provide stronger typing for backend creation. >> +# TODO This command effectively bypasses QAPI completely due to its >> +# "additional arguments" business. It shouldn't have been added to >> +# the schema in this form. It should be qapified properly, or >> +# replaced by a properly qapified command. > > I like that you are getting rid of the sentence about strong typing > (since flat unions ARE strongly typed) and instead couching it in terms > of an incomplete qapi description. I think we may even be able to come > up with a way to express it without needing a new command, and without > breaking back-compat, once we figure out how to pass unions as a command > argument. > > In fact, let's suppose we do have a new union type, NetdevInfo, as well > as a way to flag that a given command will be called by the marshaler > using JUST a pointer to the struct, rather than as a list of parameters > to members of the struct. > > { 'enum': 'NetdevTypes', [ ... ] } > { 'struct': 'NetdevInfoBase', 'data': > { 'type': 'NetdevTypes', 'id': 'str' } } > { 'union': 'NetdevInfo', 'base': 'NetdevInfoBase', > 'discriminator': 'type', 'data': { ... } } > { 'command': 'netdev_add', 'boxed': true, > 'data': 'NetdevInfo' } > > resulting in the marshaller calling > > void qmp_netdev_add(NetdevInfo *data, Error **errp) > > > Here, 'boxed' is an optional key to each 'command', defaulting to false; > but when true, 'data' is passed in a box to the command called by the > marshaler, instead of broken out into fields. 'boxed' must be true for > unions, but may be true elsewhere. Matches my plans. Note existing type NetClientOptions, used internally. I hope we can avoid duplicating it and all its Netdev*Options types. > And if we have that, then OTHER commands that have complained about > super-long parameter lists when their struct is exploded into a > parameter list could also use this mechanism, to write their handler in > terms of a single pointer to the struct. Yes. > Enough rambling about stuff for the future. This patch is fine as-is; > Reviewed-by: Eric Blake Thanks!