From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axFVv-0005OF-J7 for qemu-devel@nongnu.org; Mon, 02 May 2016 11:12:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axFVj-00035j-2b for qemu-devel@nongnu.org; Mon, 02 May 2016 11:11:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axFVi-00033z-RD for qemu-devel@nongnu.org; Mon, 02 May 2016 11:11:47 -0400 References: <1461903820-3092-1-git-send-email-eblake@redhat.com> <1461903820-3092-8-git-send-email-eblake@redhat.com> <87lh3som6o.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <57276E26.5020006@redhat.com> Date: Mon, 2 May 2016 09:11:34 -0600 MIME-Version: 1.0 In-Reply-To: <87lh3som6o.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0E235nH95pT9mo6ItxGDoIUnncXpol5B9" Subject: Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, famz@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --0E235nH95pT9mo6ItxGDoIUnncXpol5B9 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/02/2016 03:15 AM, Markus Armbruster wrote: > Title: s/json/JSON/ >=20 > Eric Blake writes: >=20 >> We have several places that want to go from qapi to JSON; right now, >> they have to create an intermediate QObject to do the work. That >> also has the drawback that the JSON formatting of a QDict will >> rearrange keys (according to a deterministic, but unpredictable, >> hash), when humans have an easier time if dicts are produced in >> the same order as the qapi type. >=20 > There's a drawback, though: more code. >=20 > Could the JSON output visitor replace the QMP output visitor? Hmm. As written here, the JSON output visitor _reuses_ the QMP output visitor, for outputting an 'any' object. Maybe the QMP output visitor could do a virtual visit through the JSON visitor, though (as in rather than directly outputting to JSON, it instead opens a JSON visitor under the hood, for some recursion when doing an 'any' visit). I can try it as followup patches, but don't want to make the original checkin any more complex than it has to be. >=20 > Aside: the QMP visitors are really QObject visitors. Yeah, particularly since they are also used by QGA. Is it worth renaming them? >> +/* >> + * The JSON output visitor does not accept Infinity or NaN to >> + * visit_type_number(). >> + */ >> +JsonOutputVisitor *json_output_visitor_new(void); >> +void json_output_visitor_cleanup(JsonOutputVisitor *v); >> +void json_output_visitor_reset(JsonOutputVisitor *v); >=20 > Hmm. Why is "reset" not a Visitor method? >=20 > I think this would let us put the things enforced by your "qmp: Tighten= > output visitor rules" in the Visitor contract. I thought about that, and now that you've mentioned it, I'll probably give it a try (that is, make visit_reset() a top-level construct that ALL visitors must support, rather than just qmp-output and json-output). >> + >> +#include "qemu/osdep.h" >> +#include "qapi/json-output-visitor.h" >> +#include "qapi/visitor-impl.h" >> +#include "qemu/queue.h" >> +#include "qemu-common.h" >=20 > qemu/queue.h and qemu-common.h are superfluous. Rebase churn, I first wrote the patches before the header cleanups. Will fix. >> + >> +static void json_output_name(JsonOutputVisitor *jov, const char *name= ) >=20 > This is called for every value, by its visit_start_FOO() or > visit_type_FOO() function. Quote visitor.h: >=20 > * The @name parameter of visit_type_FOO() describes the relation > * between this QAPI value and its parent container. When visiting > * the root of a tree, @name is ignored; when visiting a member of an > * object, @name is the key associated with the value; and when > * visiting a member of a list, @name is NULL. >=20 > Aside: it should mention visit_start_FOO() in addition to > visit_type_FOO(). >=20 Separate cleanup, but sounds useful. I can add it. >> +{ >> + if (!jov->str) { >> + jov->str =3D qstring_new(); >=20 > This happens on the first call after creation or reset of jov. >=20 > If you call json_output_get_string() after an empty visit, it chokes on= > null jov->str. Could be declared a feature. The Visitor contract is > silent on it, but the QMP output visitor rejects it since "qmp: Tighten= > output visitor rules". I think feature, so yes, I should probably make the Visitor contract explicit that at least something has to be visited via visit_type_FOO() or visit_start_XXX(). >=20 > Regardless: why not create jov->str in json_output_visitor_new(), and > truncate it in json_output_visitor_reset()? >=20 > To retain the "feature", you'd assert qstring_get_length(jov->str). Sounds reasonable. =2E.. >> +static void json_output_end_list(Visitor *v) >> +{ >> + JsonOutputVisitor *jov =3D to_jov(v); >> + assert(jov->depth); >> + jov->depth--; >> + qstring_append(jov->str, "]"); >> + jov->comma =3D true; >> +} >=20 > The nesting checks are less thorough than the QMP visitor's, because we= > don't use a stack. Okay. And at times, I've debated about giving qapi-visit-core.c a stack, if only to centralize some of the sanity checking for all visitors rather than just the particular visitors that need a stack. >> +static void json_output_type_any(Visitor *v, const char *name, QObjec= t **obj, >> + Error **errp) >> +{ >> + JsonOutputVisitor *jov =3D to_jov(v); >> + QString *str =3D qobject_to_json(*obj); >> + assert(str); >=20 > Can't happen. Can't happen now, but COULD happen if we teach qobject_to_json() to fail on an attempt to visit Inf or NaN as a number (since that is not valid JSON). Then again, if we teach it to fail, we'd want to add an Error parameter. May also be impacted by how I refactor detection of invalid numbers in response to your comments on 4/18. Should I keep or drop the assert? >> +/* Finish building, and return the resulting string. Will not be NULL= =2E */ >> +char *json_output_get_string(JsonOutputVisitor *jov) >> +{ >> + char *result; >> + >> + assert(!jov->depth); >> + result =3D g_strdup(qstring_get_str(jov->str)); >> + json_output_visitor_reset(jov); >=20 > Could avoid the strdup() if we wanted to. Needs a way to convert > jov->str to char * destructively, like g_string_free() can do for a > GString. Your choice. May be a nice pre-req patch to add; not sure if there are any other places already in tree that would benefit from it. >> + for (i =3D 0; i < ENUM_ONE__MAX; i++) { >> + visit_type_EnumOne(data->ov, "unused", &i, &error_abort); >> + >> + out =3D json_output_get_string(data->jov); >> + g_assert(*out =3D=3D '"'); >> + len =3D strlen(out); >> + g_assert(out[len - 1] =3D=3D '"'); >> + tmp =3D out + 1; >> + out[len - 1] =3D 0; >> + g_assert_cmpstr(tmp, =3D=3D, EnumOne_lookup[i]); >> + g_free(out); >=20 > Unlike in test-qmp-output-visitor.c, you don't need > qmp_output_visitor_reset() here, because json_output_get_string() does > it automatically. >=20 > Is the difference between json_output_get_string() and > qmp_output_get_qobject() a good idea? No, and it probably means I have a bug for NOT requiring the reset. >> + output_visitor_test_add("/visitor/json/any", test_visitor_out_any= ); >> + output_visitor_test_add("/visitor/json/union-flat", >> + test_visitor_out_union_flat); >> + output_visitor_test_add("/visitor/json/alternate", >> + test_visitor_out_alternate); >> + output_visitor_test_add("/visitor/json/null", test_visitor_out_nu= ll); >> + >> + g_test_run(); >> + >> + return 0; >> +} >=20 > This is obviously patterned after test-qmp-output-visitor.c. Should we= > link the two with comments, to improve our chances of them being kept i= n > sync? Sure. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --0E235nH95pT9mo6ItxGDoIUnncXpol5B9 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/ iQEcBAEBCAAGBQJXJ24mAAoJEKeha0olJ0NqMg4IAKfUI4MSf9RACqXY1S9irtzw 9ovsA2xtMih/YoDLnzVN9jGxTIyeanw/IFYqcFflAPA0WmEcmjoJPBWe8jpFH1Dt W4IpMv3CSyHnEJ2H8W13pr4WSf1UV9jhweZSJccBkohcUbKMseRRf8KBOdi39SlL PlP8DrKpNtSXMmrOIIy/KZhKhltJBDjbgVgPxy9TXMk2BA8u/504ynuukzaXcNmA vsmm59xHg3rqEihHK7x5kVpLV7iP5MoRTFpNulBFxra6JfWyzuHH93oYw1C4Kvhk sEPSUeyhQWONCKKuBa9wnmTdud7PvRXya3dZEme4ii5Ldc5FVI23sC9uv7th2DM= =qJN5 -----END PGP SIGNATURE----- --0E235nH95pT9mo6ItxGDoIUnncXpol5B9--