From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOzOg-00005h-RM for qemu-devel@nongnu.org; Thu, 28 Jan 2016 22:06:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOzOb-0005Ji-Rs for qemu-devel@nongnu.org; Thu, 28 Jan 2016 22:06:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38505) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOzOb-0005J4-Ji for qemu-devel@nongnu.org; Thu, 28 Jan 2016 22:06:49 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-21-git-send-email-eblake@redhat.com> <87wpr3dof4.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56AAD747.9000702@redhat.com> Date: Thu, 28 Jan 2016 20:06:47 -0700 MIME-Version: 1.0 In-Reply-To: <87wpr3dof4.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="RXJF6U7lmsh6u51nInVOUmNQG7HGR3get" Subject: Re: [Qemu-devel] [PATCH v9 20/37] qmp: Don't abuse stack to track qmp-output root 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) --RXJF6U7lmsh6u51nInVOUmNQG7HGR3get Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/21/2016 06:58 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The previous commit documented an inconsistency in how we are >> using the stack of qmp-output-visitor. Normally, pushing a >> single top-level object puts the object on the stack twice: >> once as the root, and once as the current container being >> appended to; but popping that struct only pops once. However, >> qmp_ouput_add() was trying to either set up the added object >> as the new root (works if you parse two top-level scalars in a >> row: the second replaces the first as the root) or as a member >> of the current container (works as long as you have an open >> container on the stack; but if you have popped the first >> top-level container, it then resolves to the root and still >> tries to add into that existing container). >> >> Fix the stupidity by not tracking two separate things in the >> stack. Drop the now-useless qmp_output_first() while at it. >> >> Saved for a later patch: we still are rather sloppy in that >> qmp_output_get_object() can be called in the middle of a parse, >> rather than requiring that a visit is complete. >> >> + switch (qobject_type(cur)) { >> + case QTYPE_QDICT: >> + assert(name); >> + qdict_put_obj(qobject_to_qdict(cur), name, value); >> + break; >> + case QTYPE_QLIST: >> + qlist_append_obj(qobject_to_qlist(cur), value); >> + break; >> + default: >> + g_assert_not_reached(); >=20 > We usually just abort(). But there are definitely existing examples, and it is a bit more self-documenting. >> >> @@ -230,7 +205,9 @@ static void qmp_output_type_any(Visitor *v, const = char *name, QObject **obj, >> /* Finish building, and return the root object. Will not be NULL. */ >> QObject *qmp_output_get_qobject(QmpOutputVisitor *qov) >> { >> - QObject *obj =3D qmp_output_first(qov); >> + /* FIXME: we should require that a visit occurred, and that it is= >> + * complete (no starts without a matching end) */ >=20 > I agree the visit must complete before you can retrieve the value. >=20 > I think there are two sane ways to recover from errors: >=20 > 1. Require the client to empty the stack by calling the necessary end > methods. >=20 > 2. Allow the client to reset or destroy the visitor without calling end= > methods. >=20 > *This* visitor would be fine with either. I guess the others would be > fine, too. So it's a question of interface design. >=20 > I'm currently leaning towards 2, because "you must do A, B and C before= > you can destroy this object" would be weird. What do you think? Patches later in the series revisit the question, and adding a reset also makes it more interesting to be able to reset at any point in a partial visit. For _this_ patch, I think we're okay, but this is probably the cutoff for what I'm sending in the first round of v10, saving all the trickier stuff for later. >=20 >> + QObject *obj =3D qov->root; >> if (obj) { >> qobject_incref(obj); >> } else { >> @@ -248,16 +225,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor= *v) >> { >> QStackEntry *e, *tmp; >> >> - /* The bottom QStackEntry, if any, owns the root QObject. See the= >> - * qmp_output_push_obj() invocations in qmp_output_add_obj(). */ >> - QObject *root =3D QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_fir= st(v); >> - >=20 > If we require end methods to be called, the stack must be empty here, > rendering the following loop useless. Yeah, an interesting observation that will affect what I do in 24/37. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --RXJF6U7lmsh6u51nInVOUmNQG7HGR3get 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/ iQEcBAEBCAAGBQJWqtdHAAoJEKeha0olJ0NqbMcH/0tZsDiLiC/sHNRTpBPlr3oT 1oJ78je+GyaZv65garovODXZY+3fb8n/AmWmcYkwVFKpoMjq+7of3uhn6+LTjk5U GDjONHOCw8qdaD5zTd5CbS7HKBg+11h/SfCygkq41l4mrx7WWEuq51X54LzzDVcP aAb6zY7kRkEJ3r8R2UmM6ryBLxLagcPhru6xj9qnYzRpFoNG0VgVvkXNFWyRvUPr 83b7fjSUTugvJLaF46OpB6wSST6zLkhVwuyqadxqvtL9E06N30SbTRJPWqlxOFnV aO9s/NjKAnOXmDFW+XU93iq3nc3ntTWmXbPvHbcqWL/GNE2FTSzSvK2ZFqEBxyg= =FptV -----END PGP SIGNATURE----- --RXJF6U7lmsh6u51nInVOUmNQG7HGR3get--