From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36367) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNhRp-0001bR-T1 for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:17:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNhRn-0000YH-Bs for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:17:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45694) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNhRn-0000YD-3i for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:17:03 -0400 Date: Thu, 14 Jul 2016 15:16:57 +0100 From: "Daniel P. Berrange" Message-ID: <20160714141657.GH18778@redhat.com> Reply-To: "Daniel P. Berrange" References: <1468468228-27827-1-git-send-email-eblake@redhat.com> <1468468228-27827-17-git-send-email-eblake@redhat.com> <20160714080423.GB18778@redhat.com> <578788E4.9020205@redhat.com> <20160714130327.GG18778@redhat.com> <57879BED.6090002@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <57879BED.6090002@redhat.com> Subject: Re: [Qemu-devel] [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optionally do string conversion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, armbru@redhat.com, Michael Roth , Andreas =?utf-8?Q?F=C3=A4rber?= On Thu, Jul 14, 2016 at 08:04:29AM -0600, Eric Blake wrote: > On 07/14/2016 07:03 AM, Daniel P. Berrange wrote: > > >>> I'm not really a huge fan of the approach there. IMHO the > >>> input visitor should strictly operate in "all native types" > >>> or "all string types" mode. The way you've done it here > >>> means that users can actually mix & match strings vs native > >>> types for each individual parameter :-( > >>> > >>> IMHO, I'd suggest you try to parse netdev_add args with > >>> it in "all native types" mode, if that fails, then re-parse > >>> in "all string types" mode. > >> > >> Sadly, this idea won't work. Without this series, the existing QMP > >> command 'netdev_add' takes mixed integers and strings in a single call, > >> by virtue of converting QObject into QemuOpts and back into QObject. > > > > Ok, so lemme check I understand. The original QObject has properties > > which are integers, but with existing code QMP allows them to be passed > > as strings or ints, converts them to QemuOpts which makes them all > > strings and converts them back to QObject leaving them all as strings. > > Yes, the existing code (using 'gen':false) just passes an entire QObject > to hand-written code; the hand-written code converted everything to > QemuOpts (which flattens both integers and strings into options), then > expanded QemuOpts back into QObject (strings-only). > > > > > You've now got rid of the QObject -> QemuOpts conversion, so we're > > not string-ifying everything, and so have to cope with arbitrary > > mix directly. > > Yes. > > > > >> Forcing the parser to allow only integers or only strings would reject > >> current uses of netdev_add, and we don't yet have a good idea whether > >> any existing clients were depending on that behavior. > >> > >> Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse > >> is by setting a single flag which controls which visitor constructor is > >> used. Your idea of parsing once with integer-only, then falling back to > >> parse a second time with string-only, would complicate the generator to > >> have to create two different visitors, rather than passing a single > >> boolean parameter through to the single visitor constructor. > > > > So I don't think a simple boolean flag is desirable, as we have 3 distinct > > scenarios: > > > > 1. Strongly typed only > > 2. String conversion only > > 3. Strongly typed, with string conversion fallback > > > > My current patch provides 1 + 2, your alternative patch provides 1 + 3. > > If we use option 3, in cases which only actually need option 2, then we > > are potentially introducing more scenarios where arbitrarily mixed > > types are accepted. IOW, I think we must implement 1, 2 and 3 and avoid > > using 3 except where absolutely needed. > > 3 is a superset of 2, and your command-line conversion is the only case > where we can achieve 2. That is, code dealing with QMP can only choose > between 1 and 3, based on whether the QAPI .json file used > 'autocast':true for back-compat reasons (the only candidates are > 'netdev_add' and 'device_add'). And code dealing with command line > parsing can only choose 2 (QemuOpts is string-only), but parsing > string-only via 2 is no different than the result achieved from parsing > strongly-typed with string fallback via 3. > > I still don't buy the fact that we need a string-only parser at the > moment, but it would not be hard to change the 'bool autocast' into a > tri-state enum, and then make the implementation specifically honor 1, > 2, or 3 based on the enum value. FYI, I just copied you on a patch that enables us to easily support all 3 options. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|