From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36460) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcgfQ-0000AU-Nj for qemu-devel@nongnu.org; Thu, 17 Sep 2015 17:24:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcgfN-0005ca-DA for qemu-devel@nongnu.org; Thu, 17 Sep 2015 17:24:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43442) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcgfN-0005bk-5M for qemu-devel@nongnu.org; Thu, 17 Sep 2015 17:24:29 -0400 References: <828b3ad8c9f32d5938f39068328d0794cfd9abf4.1441627176.git.DirtY.iCE.hu@gmail.com> From: Eric Blake Message-ID: <55FB2F86.1040407@redhat.com> Date: Thu, 17 Sep 2015 15:24:22 -0600 MIME-Version: 1.0 In-Reply-To: <828b3ad8c9f32d5938f39068328d0794cfd9abf4.1441627176.git.DirtY.iCE.hu@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lxMHOUnGStHGSmwVUBNU9aim2WDhsnxcn" Subject: Re: [Qemu-devel] [PATCH 7/7] qapi: support nested structs in OptsVisitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?S8WRdsOhZ8OzLCBab2x0w6Fu?= , qemu-devel@nongnu.org Cc: Markus Armbruster , Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --lxMHOUnGStHGSmwVUBNU9aim2WDhsnxcn Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/07/2015 06:14 AM, K=C5=91v=C3=A1g=C3=B3, Zolt=C3=A1n wrote: > The current OptsVisitor flattens the whole structure, if there are same= > named fields under different paths (like `in' and `out' in `Audiodev'),= > the current visitor can't cope with them (for example setting > `frequency=3D44100' will set the in's frequency to 44100 and leave out'= s > frequency unspecified). >=20 > This patch fixes it, by always requiring a complete path in case of > nested structs. Fields in the path are separated by dots, similar to C= > structs (without pointers), like `in.frequency' or `out.frequency'. >=20 > You must provide a full path even in non-ambiguous cases. The qapi > flattening commits hopefully ensures that this change doesn't create > backward compatibility problems. It's the "hopefully" that worries me. If I understand correctly, prior to this series, we have several instances of qapi simple unions, where a member is a substruct when converting to QObject. Conceptually: { 'struct':'Sub', 'data':{ 'c':'int' } } { 'union':'Main', 'data':{ 'X':'Sub' } } would accept the following QDict: { "type":"X", "data":{ "c":1 } } and directly translating that to QemuOpts would be spelled: -opt type=3DX,data.c=3D1 but we wanted shorthand: -opt type=3DX,c=3D1 So, the "flattening" that opts-visitor does is what allows "c" instead of "data.c" to refer to a nested member of a union. It worked as long as 'c' appeared only once per visit of the overall qapi type (multiple branches of the union may have 'c', but for a given union branch, 'c' appears only once no matter what depth of substructs it is reached throug= h). Question: do we actually ALLOW the user to specify data.c, or do we ONLY accept the shorthand? This series then proceeds to teach opts-visitor how to handle flat unions. Converting the above example to a flat union is done by: { 'enum':'Foo', 'data':['X'] } { 'struct':'Base', 'data':{ 'type':'Foo' } } { 'struct':'Sub', 'data':{ 'c':'int' } } { 'union':'Main', 'base':'Base', 'discriminator':'type', 'data':{ 'X':'Sub' } } which now accepts the following QDict: { "type":"X", "c":1" } (note the loss of the nested 'data' struct), and directly translating that to QemuOpts would be spelled: -opt type=3DX,c=3D1 which exactly matches the shorthand that we previously accepted. It completely gets rid of the data.c longhand, but I don't know if we accepted that in the first place. So, the next question is whether we still need flattening. If we no longer have any simple unions parsed by QemuOpts, then the flattening no longer helps us. Furthermore, you have a case in audio where flattening hurts; the QDict: { "in":{ "frequency":4400 }, "out":{ "frequency":4400 } } has unambiguous paths 'in.frequency' and 'out.frequency', but the shorthand 'frequency' could now match two separate places in the struct. Now, let's look at what your testsuite changes say about this patch: > @@ -271,6 +299,12 @@ main(int argc, char **argv) > add_test("/visitor/opts/i64/range/2big/full", &expect_fail, > "i64=3D-0x8000000000000000-0x7fffffffffffffff"); > =20 > + /* Test nested structs support */ > + add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=3D= 13"); You are saying that an unqualified member no longer resolves (that is, when nesting is in place, we MUST spell it by the long name); > + add_test("/visitor/opts/nested/both", &expect_both, > + "sub0.nint=3D13,sub1.nint=3D17"); > + add_test("/visitor/opts/nested/sub0", &expect_sub0, "sub0.n= int=3D13"); > + add_test("/visitor/opts/nested/sub1", &expect_sub1, "sub1.n= int=3D13"); and that using qualified names gets at the nested struct members as desired. Furthermore, if we truly have converted ALL qapi unions to flat unions, and if qapi unions were the ONLY reason that we had flattening in the first place, and if we did NOT accept longhand 'data.c=3D1' for the flattened shorthand of 'c=3D1' within a simple union= branch, then it looks like you are 100% backwards-compatible. But that's quite a few things to verify. Also, I'm not sure if your visitor approaches the change of no longer flattening things in the most efficient manner (I still haven't studied the rest of the patch closely).= I'd like to defer reviewing this until the qapi patch queue is not quite so long, but the premise behind it is appealing. However, I strongly recommend that the commit message be improved to cover some of the background that I spelled out here, as well as offering more proof than just "hopefully", and an analysis of whether we are breaking any longhand data.c=3D1 option spellings. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --lxMHOUnGStHGSmwVUBNU9aim2WDhsnxcn 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/ iQEcBAEBCAAGBQJV+y+GAAoJEKeha0olJ0NqT4wH/0OSrT51mcmJuy32rc15N2UQ 6zMGg3xPhvavZ4sMtvIF7v1JzyOD8W4XbH11d1JtCUW6eTZjuG8nQOQ/2e/QptK2 xlWNPjbPjb7tuV2KS/oC2b81U923tQ26Rento5PLOIEAy1ZbYfCNTDDzMccXMOuc pCkxGGMG0Ihf0WLgbLBTIyqo5Czl0uPpiBWI4338JPn8JWtELubrZueCEa1pj8g2 4/w2a6a4IPoU9xFNAwefEbKX/e1rXRmoZPnu0tSskL57t1/r8e/vXkA/CCMIILSV hJcfc62EodMyDAoWyN/PyEYEiHpVIx5t7dNXPzOsF/uX472/f/ueTrEz7ZGvD/o= =c59L -----END PGP SIGNATURE----- --lxMHOUnGStHGSmwVUBNU9aim2WDhsnxcn--