From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avEIl-00038P-HK for qemu-devel@nongnu.org; Tue, 26 Apr 2016 21:30:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avEIi-0002Z6-6u for qemu-devel@nongnu.org; Tue, 26 Apr 2016 21:30:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45986) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avEIh-0002Z2-Vy for qemu-devel@nongnu.org; Tue, 26 Apr 2016 21:30:00 -0400 References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-14-git-send-email-eblake@redhat.com> <877ffzutw8.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <57201615.4040801@redhat.com> Date: Tue, 26 Apr 2016 19:29:57 -0600 MIME-Version: 1.0 In-Reply-To: <877ffzutw8.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qj46lFIr2Q8Pef4b4FnSCVfXJKSVnDwxI" Subject: Re: [Qemu-devel] [PATCH v14 13/19] qmp: Tighten output visitor rules List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --qj46lFIr2Q8Pef4b4FnSCVfXJKSVnDwxI Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/15/2016 03:02 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Add a new qmp_output_visitor_reset(), which must be called before >> reusing an exising QmpOutputVisitor on a new root object. Tighten >> assertions to require that qmp_output_get_qobject() can only be >> called after pairing a visit_end_* for every visit_start_* (rather >> than allowing it to return a partially built object), and that it >> must not be called unless at least one visit_type_* or visit_start/ >> visit_end pair has occurred since creation/reset (the accidental >> return of NULL fixed by commit ab8bf1d7 would have been much >> easier to diagnose). >> >> Also, check that we are encountering the expected object or list >> type (both during visit_end*, and also by validating whether 'name' >> was NULL - although the latter may need to change later if we >> improve error messages by always passing in a sensible name). >> This provides protection against mismatched push(struct)/pop(list) >> or push(list)/pop(struct), similar to the qmp-input protection >> added in commit bdd8e6b5. >> >> Signed-off-by: Eric Blake >=20 > As written, the commit message makes me wonder why we add > qmp_output_visitor_reset() in the same commit. I think the reason is > the tightened rules make it necessary. The commit message could make > that clearer by explaining the rule changes first, then point out we > need a reset to comply with the rules. I think I'll try splitting the addition of qmp_output_visitor_reset() into a separate patch. >> @@ -93,6 +92,9 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov= , const char *name, >> qdict_put_obj(qobject_to_qdict(cur), name, value); >> break; >> case QTYPE_QLIST: >> + /* FIXME: assertion needs adjustment if we fix visit-core= >> + * to pass "name.0" style name during lists. */ >=20 > visit-core merely passes through whatever name it gets from the client.= > Thus, saying we 'fix visit-core to pass "name.0"' is a bit misleading. > What we'd do is change it to require "name.0", then update its users to= > comply. Maybe it's not too inaccurate - the only callers are the generated visit_type_FOOList() functions, but having a common "name.%d" generator in the core may be easier than bloating each generated visit_type_FOOList= =2E >=20 > Moreover, this is a note, not a FIXME: nothing is broken here. The > closest we get to "broken" are the bad error messages, but they're > elsewhere. >=20 > I'd simply drop the comment. But this solution nicely sidesteps the "how will we fix error messages", so I've dropped the comment. >=20 >> + assert(!name); >=20 > PATCH 08 made this part of the contract. It also added a bunch of > contract-checking assertions. Should this one be in PATCH 08, too? Well, it's only weakly part of the contract unless (until?) we fix callers/core to pass in "name.0", and then the assert would trigger. However, checking the assertion in patch 8 is harder, without making the core track whether it is currently in a list or struct visit (that is, the only place where we know whether 'name' should be NULL or not is where we've tracked a stack of our current visit_start_* calls; but the core is not tracking a stack because that would be redundant with the stacks in the qmp visitors). So for now I think I'll just keep it here. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -139,6 +139,7 @@ static void test_visitor_out_enum(TestOutputVisito= rData *data, >> @@ -455,6 +460,7 @@ static void test_visitor_out_alternate(TestOutputV= isitorData *data, >> qapi_free_UserDefAlternate(tmp); >> qobject_decref(arg); >> >> + qmp_output_visitor_reset(data->qov); >> tmp =3D g_new0(UserDefAlternate, 1); >> tmp->type =3D QTYPE_QDICT; >> tmp->u.udfu.integer =3D 1; >=20 > How did you find the places that now need qmp_output_visitor_reset()? Ran the test, found what asserted, and added a reset() to make the test pass again. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --qj46lFIr2Q8Pef4b4FnSCVfXJKSVnDwxI 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/ iQEcBAEBCAAGBQJXIBYVAAoJEKeha0olJ0Nq7loIAKcaBEqW0I8F3Qy8WPyzlJsv a8fbVr+kAlY1PhPrIetRkpJrc+Zfk/vEQKDAV5VFtxRQu/gTOISSseQdmGxxkIUb +lkSJ1vJ8ApEdEgJujeHWhJGyEbk8sgFth38XC2qDLozFnlombhp6qUc27HCW34c tMHRzThQ+6tiRjGWoXxcRZkWcpUmUOES07/5XgKh2EdJhjnaIQMBbrU440zQQxLH eaGkin9xpbQjFVK2IdHrAfU1feyH8crktlezO29LzTtteMd2xab1N9chX84tiJUV d0l5Vpzpl4dNK0XOvxFB1SVNwH/r+PqyzzYBEN0x1YHp/RLIr1d1I8u+iexn+5g= =LYQj -----END PGP SIGNATURE----- --qj46lFIr2Q8Pef4b4FnSCVfXJKSVnDwxI--