From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTIZ2-0007k2-NY for qemu-devel@nongnu.org; Tue, 09 Feb 2016 19:23:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTIYz-0003Kj-Hs for qemu-devel@nongnu.org; Tue, 09 Feb 2016 19:23:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59101) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTIYz-0003Jg-9T for qemu-devel@nongnu.org; Tue, 09 Feb 2016 19:23:21 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-22-git-send-email-eblake@redhat.com> <87vb6m7l25.fsf@blackfin.pond.sub.org> <56A17830.7010305@redhat.com> <87mvrx24fa.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56BA82F7.1020202@redhat.com> Date: Tue, 9 Feb 2016 17:23:19 -0700 MIME-Version: 1.0 In-Reply-To: <87mvrx24fa.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jN7bCxgDferbikD0chnt1J0pdSa9CbVla" Subject: Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --jN7bCxgDferbikD0chnt1J0pdSa9CbVla Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/22/2016 05:18 AM, Markus Armbruster wrote: > Please think twice before from growing the QAPI patch queue further. W= e > really, really need to clear it. A TODO comment would be welcome, > though. Yes, especially with 2.6 soft freeze fast approaching. >>>> +/** >>>> + * Prepare to visit an implicit struct. >>>> + * Similar to visit_start_struct(), except that an implicit struct >>>> + * represents a subset of keys that are present at the same nesting= level >>>> + * of a common object but as a separate qapi C struct, rather than = a new >>>> + * object at a deeper nesting level. >>> >>> I'm afraid this is impenetrable. >>> >>> I tried to refresh my memory on visit_start_implicit_struct()'s purpo= se >>> by reading code, but that's pretty impenetrable, too. >> >> >> Suggestions on how to better word it are welcome. I'm basically tryin= g >> to describe that this function marks the start of a new C struct used = as >> a sub-object while still conceptually parsing the same top-level QAPI >> struct. >=20 > Thinking aloud... >=20 > QAPI defines both C data types and a QMP wire format. >=20 > The work of mapping between the two is split between generated visitor > functions and the QMP visitors. Roughly, the visitor functions direct > the mapping of the tree structure, and the QMP visitors take care of th= e > leaves. >=20 > The QAPI tree structure and the QMP tree structure are roughly the same= =2E > Exception: some structs are inlined into a parent struct in QMP, but > stored as a separate, boxed struct in QAPI. We call them "implicit" > structs. We got rid of one case (base structs), but we still got two > (flat union and alternate members). I dislike the exception, but we > need to document what we've got. >=20 > It's mostly handled by the visitor functions, but two visitors need to > hook into their handling, because they allocate / free the boxed struct= : > the QMP input visitor and the dealloc visitor. >=20 > The QMP output visitor doesn't. The effect is that the members of the > implicit struct get inlined into the explicit struct that surrounds it.= >=20 > I oversimplified when I said "and a QMP wire format": since we acquired= > string and QemuOpts visitors, we also have a string and QemuOpts format= =2E > Both are partial: they don't support all of QAPI. >=20 > However, these formats aren't independent of the QMP wire format. Sinc= e > we use the same visitor functions, and the visitor functions map the > tree structure, the tree structure gets mapped just like for QMP. > Almost theoretical, because these visitors don't support most of the > structure. >=20 > If we wanted a format that doesn't inline implicit structs, the visitor= > would want to implement visit_start_implicit_struct() and > visit_end_implicit_struct() just like visit_start_struct() and > visit_end_struct(). Differences: >=20 > * start_implicit_struct() lacks the @name parameter. >=20 > * end_implicit_struct() lacks the @errp now. Later in this series, thi= s > becomes: there is no check_implicit_struct(). >=20 > Not inlining implicit structs seems impossible with the current API. > I'm *not* asking for a change that makes it possible. Instead, my poin= t > is: the inlining of implicit structs is baked into the visitor > interfaces. >=20 > I'm afraid I can't turn this into a reasonable function comment without= > first amending the introduction comment to cover tree structure mapping= =2E > Ugh. >=20 > Crazy thought: unboxing the implicit struct should make this interface > wart go away. If we commit to do that later, we can "solve" our > documentation problem the same way as for visit_start_union(): FIXME > should not be needed. I _want_ to get rid of the boxing. But as it is not in my current queue of pending patches, it will have to wait until the current queue is flushed; so I'm going for documenting it with FIXMEs for now. Basically, our current flat union representation is: struct Union { Type tag; union { Subtype1 *one; Subtype2 *two; } u; }; which requires two malloc's to completely populate the struct, and where we access union->u.one->member, or pass union->u.one to a function taking Subtype1*. But it _should_ be: struct Union { Type tag; union { Subtype1 one; Subtype2 two; } u; }; where a single malloc is sufficient, and where we access union->u.one.member, or pass &union->u.one to a function taking Subtype1*= =2E It's a tree-wide conversion; and may be easier if done in stages (fix the generator to take a temporary boolean flag on whether a particular structure uses inline or boxing, then a series of patches adding that flag to a few QMP commands at a time, then a final patch to clear out the temporary flag support) rather than all at once. I'm not sure how much Coccinelle will help, because I specifically haven't started the conversion work until after we can get the current backlog flushed. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --jN7bCxgDferbikD0chnt1J0pdSa9CbVla 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/ iQEcBAEBCAAGBQJWuoL3AAoJEKeha0olJ0Nqa6UH/22hM+RSCJBP3rbquOUFSMby X995d3GB++eBx3PCUul0s9p2FtOnNOI4En/qL/5+zif8xg29HLmFn4QU6Ziay2HZ gNA+Cvhf3cLATZR6vjpXurx4MqxO1Wu2N/joqHSHPgSDNNM7qHHZPszgVd5wRB8c oN0QvFWFjFm1eCYlhIoYrj23XzNrf6VUnPvKwlwl+ueg+31QCVTREFVS8bYvyZjG yf49Tjdq1S9FG7QaavgpgYKBq3vai0xyNR5SIGviAQIoYr8E3evkkLasnA5VsvjJ 1pYVISLp3l64LGTsemMWFCMacF4q4nLKFpS4D6arxm+WGgBzoYIOWo2lPgWj43I= =trE9 -----END PGP SIGNATURE----- --jN7bCxgDferbikD0chnt1J0pdSa9CbVla--