From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXy8J-0002Dj-OH for qemu-devel@nongnu.org; Fri, 04 Sep 2015 17:02:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXy8F-0001jY-A6 for qemu-devel@nongnu.org; Fri, 04 Sep 2015 17:02:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXy8F-0001ia-2D for qemu-devel@nongnu.org; Fri, 04 Sep 2015 17:02:47 -0400 References: <7dcbdc8169de6b0090aa1ba89e7426fb3cd0e6cf.1440171025.git.DirtY.iCE.hu@gmail.com> From: Eric Blake Message-ID: <55EA06F1.2000602@redhat.com> Date: Fri, 4 Sep 2015 15:02:41 -0600 MIME-Version: 1.0 In-Reply-To: <7dcbdc8169de6b0090aa1ba89e7426fb3cd0e6cf.1440171025.git.DirtY.iCE.hu@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mowlQ4DQdC6aLwCRpD6iluxNclR3aD12T" Subject: Re: [Qemu-devel] [PATCH v2 03/49] qapi: convert NumaOptions into a flat union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?S8WRdsOhZ8OzLCBab2x0w6Fu?= , qemu-devel@nongnu.org Cc: Markus Armbruster , Gerd Hoffmann , Eduardo Habkost This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --mowlQ4DQdC6aLwCRpD6iluxNclR3aD12T Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08/21/2015 09:36 AM, K=C5=91v=C3=A1g=C3=B3, Zolt=C3=A1n wrote: > Signed-off-by: K=C5=91v=C3=A1g=C3=B3, Zolt=C3=A1n The subject line is a one-line "what", and the commit body should be the "why" - all but the most trivial "what" deserve a non-empty commit body. In particular, I think you NEED to mention in the commit body that the conversion does not strictly follow the QMP wire equivalency between simple and flat unions as spelled out in docs/qapi-code-gen.txt, but that this is okay because we do not have any QMP commands that use the NumaOptions type to begin with (so there is no observable change to any commands sent over QMP). Meanwhile, mention that the change is made to convert the generated C code into something that will ultimately be easier to work with. > --- > numa.c | 2 +- > qapi-schema.json | 47 ++++++++++++++++++++++++++++++++++++----------- > 2 files changed, 37 insertions(+), 12 deletions(-) >=20 >=20 > +++ b/numa.c > @@ -227,7 +227,7 @@ static int parse_numa(void *opaque, QemuOpts *opts,= Error **errp) > } > =20 > switch (object->kind) { > - case NUMA_OPTIONS_KIND_NODE: > + case NUMA_DRIVER_NODE: Now that commit 0f61af3 has landed, you'll have to rebase your patch to use 'switch (object->type) {' here. Basically, the conversion from simple to flat union now (temporarily) generates a different tag name for simple unions than for flat unions (although I have a pending patch [1] that solves that, but by using object->type everywhere). [1] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02061.html > +++ b/qapi-schema.json > @@ -3536,17 +3536,6 @@ > 'data': { '*console':'int', 'events': [ 'InputEvent' ] } } > =20 > ## > -# @NumaOptions > -# > -# A discriminated record of NUMA options. (for OptsVisitor) > -# > -# Since 2.1 > -## > -{ 'union': 'NumaOptions', > - 'data': { > - 'node': 'NumaNodeOptions' }} If we were following the equivalency documented in docs/qapi-code-gen.txt, then this part is okay (you are removing the simple union),... > ## > +# @NumaDriver > +# > +# List of possible numa drivers. > +# > +# Since: 2.5 > +## > +{ 'enum': 'NumaDriver', > + 'data': [ 'node' ] } =2E..this part is okay (you are creating a new enum, which covers all branches present in the former simple union),... > + > +## > +# @NumaCommonOptions > +# > +# Common set of numa options. > +# > +# @type: the numa driver to use > +# > +# Since: 2.5 > +## > +{ 'struct': 'NumaCommonOptions', > + 'data': { > + 'type': 'NumaDriver' } } =2E..this part is okay (you are creating a new base type, which consists of a single common member named 'type' of the new enum type),... [By the way, while the base type is currently required, I have a pending patch that would add syntax sugar to avoid it, so maybe you want to wait for my patch [2] to land? [2] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02321.html so you could write 'base': { 'type': 'NumaDriver' } directly in the flat union, instead of having to declare NumaCommonOptions] > + > +## > +# @NumaOptions > +# > +# A discriminated record of NUMA options. (for OptsVisitor) > +# > +# Since 2.1 > +## > +{ 'union': 'NumaOptions', > + 'base': 'NumaCommonOptions', > + 'discriminator': 'type', > + 'data': { > + 'node': 'NumaNodeOptions' }} =2E..but this part is missing one piece of the conversion (to be strictly= QMP compatible with the simple union, each branch of the flat union must be a NEW type that contains a single member 'data' of the old type). That is, if we were TRYING to preserve QMP compatibility, we'd need: { 'struct': 'NumaNodeOptionsWrapper', 'data': { 'data': 'NumaNodeOptions' } } { 'union': 'NumaOptions', 'base': 'NumaCommonOptions', 'discriminator': 'type', 'data': { 'node': 'NumaNodeOptionsWrapper' } } The difference on the wire is that the old simple union, or with my version of the flat union, would allow: { 'command': 'foo', 'arguments': { 'type': 'numa', 'data': { 'nodeid': 1 } } } while the new flat union as you wrote it reduces nesting: { 'command': 'foo', 'arguments': { 'type': 'numa', 'nodeid': 1 } } That said, there is another point of view to worry about, which is what C structures get generated. Here, I'm using output after Markus' v4 series is applied (since it is slightly more legible than current qemu.git mainline generated output) (and although I still have further pending patches that improve it further). The pre-patch simple union would generate: struct NumaOptions { NumaOptionsKind kind; union { /* union tag is @kind */ void *data; NumaNodeOptions *node; }; }; and your version would generate (notice the rename from 'kind' to 'type')= : struct NumaOptions { /* Members inherited from NumaCommonOptions: */ NumaDriver type; /* Own members: */ union { /* union tag is @type */ void *data; NumaNodeOptions *node; }; }; but my proposal to keep QMP compatibility would generate an incompatible layer of boxing: struct NumaOptions { /* Members inherited from NumaCommonOptions: */ NumaDriver type; /* Own members: */ union { /* union tag is @type */ void *data; NumaNodeOptionsWrapper *node; }; }; So, in conclusion, since no existing QMP command used the old type, and your choice of changes preserved the generated struct layout, it means we have no change to command line parsing and no user-visible changes to worry about. And I'm fine with this patch, once you improve the commit message and rebase to mainline: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mowlQ4DQdC6aLwCRpD6iluxNclR3aD12T 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/ iQEcBAEBCAAGBQJV6gbxAAoJEKeha0olJ0NqWfkIAI6Ke79QKOzriz9czKaivI0R zGHrueOripxl9Ck4BUTTZiaJh3z5V2ImmUphzZ1RaULlOwukl9bhWGT6zjimQWSe azg9/nhmO+OIIXt3SdUodzvmO0j8v5XbcXbmiALYIJQXI/eXCeNyrNjkFQ58yTln e240bOGjIA51xyyvgt9aN7JnP+/8+LBIPb+n5JiWSMljH3j1V9f/+EerNGdoZNiP 3eK4tiTeDcGLPJuxBNzuV18QV+hZ4/L40KlAE2eyVJrUeMVicXb7l1ypzg9Y4Hik jhMHSV2ZyTYbZMEktdlgZNjoA9IseB4VxUFnPMIEKP+BchGNrBJc3Hc0KXUPmv8= =wq6P -----END PGP SIGNATURE----- --mowlQ4DQdC6aLwCRpD6iluxNclR3aD12T--