From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNhFk-000331-0K for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:04:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNhFf-0006ef-Lx for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:04:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48791) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNhFf-0006eY-DS for qemu-devel@nongnu.org; Thu, 14 Jul 2016 10:04:31 -0400 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> From: Eric Blake Message-ID: <57879BED.6090002@redhat.com> Date: Thu, 14 Jul 2016 08:04:29 -0600 MIME-Version: 1.0 In-Reply-To: <20160714130327.GG18778@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6In9hcMDqSDu29phRfP4ahKpFT7axrW11" 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: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, armbru@redhat.com, Michael Roth , =?UTF-8?Q?Andreas_F=c3=a4rber?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6In9hcMDqSDu29phRfP4ahKpFT7axrW11 From: Eric Blake To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, armbru@redhat.com, Michael Roth , =?UTF-8?Q?Andreas_F=c3=a4rber?= Message-ID: <57879BED.6090002@redhat.com> Subject: Re: [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optionally do string conversion 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> In-Reply-To: <20160714130327.GG18778@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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). >=20 > 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. >=20 >> 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 par= se >> is by setting a single flag which controls which visitor constructor i= s >> used. Your idea of parsing once with integer-only, then falling back = to >> parse a second time with string-only, would complicate the generator t= o >> have to create two different visitors, rather than passing a single >> boolean parameter through to the single visitor constructor. >=20 > So I don't think a simple boolean flag is desirable, as we have 3 disti= nct > scenarios: >=20 > 1. Strongly typed only > 2. String conversion only > 3. Strongly typed, with string conversion fallback >=20 > 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 avoi= d > 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --6In9hcMDqSDu29phRfP4ahKpFT7axrW11 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXh5vuAAoJEKeha0olJ0NqfOYH/01KLRcrJsLPWcl/toca3aLZ 89w+jOSG9mic5EhaGCz1jY+Xih+Y0cvcWVxW/FZBgEtUoBU5TfR/OG972fvtCtfF /wSWWD54yLRLk00mbvU13LDtif1T+ucuc15eEHco5HLnyhXy+PAK6X3aqlE+8EVj kzwYPg4FY1/7bl3nzD37v9preyf/ZQ1xGgpCex+fZOZyGYES5CeZA27olafZ627R VLPnLPfknsASj1mJTE+FMYJhjdILw5c2AH0ns3ZahnNZ3Zb1Of6izVJFvETjV0cT LuEwIhC4k8nq6WNvexSMGl13bXxz/d62qkN3lS2LknWFYK+nRW/CpYVbLv4ngnU= =bJMP -----END PGP SIGNATURE----- --6In9hcMDqSDu29phRfP4ahKpFT7axrW11--