From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34762) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAK3b-0006bH-25 for qemu-devel@nongnu.org; Tue, 07 Jun 2016 12:40:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAK3Y-0000mK-O5 for qemu-devel@nongnu.org; Tue, 07 Jun 2016 12:40:46 -0400 References: <1465294275-8733-1-git-send-email-berrange@redhat.com> <1465294275-8733-5-git-send-email-berrange@redhat.com> <5756F1CC.5010408@redhat.com> <20160607162042.GU20196@redhat.com> From: Eric Blake Message-ID: <5756F904.1090804@redhat.com> Date: Tue, 7 Jun 2016 10:40:36 -0600 MIME-Version: 1.0 In-Reply-To: <20160607162042.GU20196@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Vj6NebCCKG3fm9Xu4vp0f8O26S3UfIruS" Subject: Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Kevin Wolf , Max Reitz , Markus Armbruster , Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Vj6NebCCKG3fm9Xu4vp0f8O26S3UfIruS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/07/2016 10:20 AM, Daniel P. Berrange wrote: > On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote: >> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote: >>> The current approach for pretty-printing QAPI types is to >>> convert them to JSON using the QMP output visitor and then >>> pretty-print the JSON document. This has an unfixable problem >>> that structs get their keys printed out in random order, since >>> JSON dicts do not contain any key ordering information. >>> >>> To address this, introduce a text output visitor that can >>> directly pretty print a QAPI type into a string. >>> >>> Signed-off-by: Daniel P. Berrange >>> --- >>> include/qapi/text-output-visitor.h | 73 ++++++++++++ >>> include/qapi/visitor-impl.h | 5 +- >>> include/qapi/visitor.h | 5 +- >>> qapi/Makefile.objs | 1 + >>> qapi/opts-visitor.c | 5 +- >>> qapi/qapi-dealloc-visitor.c | 4 +- >>> qapi/qapi-visit-core.c | 9 +- >>> qapi/qmp-input-visitor.c | 5 +- >>> qapi/qmp-output-visitor.c | 4 +- >>> qapi/string-input-visitor.c | 5 +- >>> qapi/string-output-visitor.c | 5 +- >> >> Why can't we enhance the existing string-output-visitor to handle stru= cts? >=20 > string-output-visitor seems to be doing something very > different from this. In particular it only ever seems > to output the values, never the field names. So if we > did enhance string-output-visitor, we'd basically have > to make all of its code conditional to output in one > style or the other style, at which point I didn't think > it was really buying us anything vs a new visitor. That is, it was always doing a top-level visit of a scalar or array of scalars, and nothing else. It may still be something that can be merged. Maybe I should take a rough shot at it, since I have ideas on how to use a common handler for name/list index (and do nothing at the top level), then the rest of each callback is independent from what name prefix, if any, was output. On the other hand, I guess the way intList is handled (compacting it into a single list, instead of each element of the list), may indeed be a reason to keep it as two visitors. >> Why do we need to list the element size twice? Or is one the size of >> the GenericList object wrapping the element? I'm still not convinced = we >> need the size of an element at this point in the visit. >=20 > 'size_t element' isn't the size of an element, it is the really > the list index - eg 1, 2, 3, 4, etc. I didn't call it 'index' > because that causes a clashing symbol, but I guess I could > have used 'idx' instead. The perils of replying to email text in the order I read it. I think I figured that out later on. And it is indeed annoying that older gcc warns about conflicts with shadowing 'index'. >> Don't we already have a util/ function for pretty-printing a size? In= >> fact, doesn't the existing StringOutputVisitor have code for doing it?= >=20 > Guess where this code was copied from - StringOutputVisitor :-) If this= > idea is amenable in general, I'll go back and cleanup this patch be bet= ter. A common helper may be nice, if we have two visitors both wanting to use = it. >> >> Oooooh, I see. You're using 'element' as the index within the array, >> not as a size. I think naming it 'idx' (or 'index', if that doesn't >> cause older compilers to barf for shadowing a function name), would ma= ke >> more sense, but it definitely highlights the need for documentation. >> Also, why does element 0 have to be special-cased? >=20 > The pattern of calling means 'next_list' is invoked /after/ > each element is visited, but we want to print out the header > /before/ each element. So I had to special case 0, and also > use the 'if(ret)' check to skip printing a header after the > last element. But if we have a common helper function (see json_output_name() in my JSON series), then that helper will be called before every element, and can easily tell whether we are in struct context (name is non-null, print it) or list context (name is NULL, print our current counter then increment it), rather than having to special case the code around the elements. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Vj6NebCCKG3fm9Xu4vp0f8O26S3UfIruS 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/ iQEcBAEBCAAGBQJXVvkEAAoJEKeha0olJ0Nqpc0H/RSc+H5W8417wyJVQ1F1XX69 NMMmmkfeM5hSsprPE37PjiugoLdiXsxEeo7n0HV6rZlDTfU/E6xzw2GxwWEMqUIP +rAfn7osrQpoCCJCa7ZPOGiYwPR4oVxnLXDHPofk1g+/DHIPCiBmlTGHmHBdSlbC n1aDdZq0NHgpDbpCvmmgdCy9agJIuCSI4r/KvBFfNvwJ9whBHbcApxNfDgC9OHcG wEeBTC7PQsnfyDFopd77wjmung/7zfHzRxfclkXn/Q1MJeaGUBxJ1DTjCRh8KjZH jdxSYsCycjWYuYJT1wl25T70sFkY6Cc61s1AbFwRELc0VaNj7TlXVxdR2JQ5gHc= =CxCY -----END PGP SIGNATURE----- --Vj6NebCCKG3fm9Xu4vp0f8O26S3UfIruS--