From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avXN2-0007Sp-SN for qemu-devel@nongnu.org; Wed, 27 Apr 2016 17:51:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avXMz-00083P-LS for qemu-devel@nongnu.org; Wed, 27 Apr 2016 17:51:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50720) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avXMz-00083I-Dr for qemu-devel@nongnu.org; Wed, 27 Apr 2016 17:51:41 -0400 References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-20-git-send-email-eblake@redhat.com> <87lh4forjm.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <5721346B.4080000@redhat.com> Date: Wed, 27 Apr 2016 15:51:39 -0600 MIME-Version: 1.0 In-Reply-To: <87lh4forjm.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="l3cjSxVUMLE3lnbHXUU2mg8lg8hbTh8W1" Subject: Re: [Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no longer return partial objects 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) --l3cjSxVUMLE3lnbHXUU2mg8lg8hbTh8W1 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/15/2016 08:49 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Returning a partial object on error is an invitation for a careless >> caller to leak memory. As no one outside the testsuite was actually >> relying on these semantics, it is cleaner to just document and >> guarantee that ALL pointer-based visit_type_FOO() functions always >> leave a safe value in *obj during an input visitor (either the new >> object on success, or NULL if an error is encountered), so callers >> can now unconditionally use qapi_free_FOO() to clean up regardless >> of whether an error occurred. >=20 > Hmm, wasn't the "assign null on error" part done in a prior patch? > Checking... no, only half of it, in PATCH 03: there, we went from "may= > store an incomplete object on error" to "store either an incomplete > object or null on error". Now we go on to just "store null on error". > Correct? Yes. I'll tweak the wording to make it clearer. >=20 >> The decision is done by enhancing qapi-visit-core to return true >> for input visitors (the callbacks themselves do not need >> modification); since we've documented that visit_end_* must be >> called after any successful visit_start_*, that is a sufficient >> point for knowing that something was allocated during start. >=20 > I find this sentence a bit confusing. Let me try: >=20 > To help visitor-agnostic code, such as the generated qapi-visit.c, > make the visit_end_FOO() return true when something was allocated. > Easily done in the visitor core, no need to change the callbacks. >=20 > But see my comments on the visit_end_FOO() inline. Reply below, where your comments are indeed worth thinking about. >=20 >> Note that we still leave *obj unchanged after a scalar-based >> visit_type_FOO(); I did not feel like auditing all uses of >> visit_type_Enum() to see if the callers would tolerate a specific >> sentinel value (not to mention having to decide whether it would >> be better to use 0 or ENUM__MAX as that sentinel). >=20 > Should this note be in PATCH 03? >=20 > The inconsistency isn't pretty, but tolerable if it simplifies things. No. Patch 03 fixed visit_start_struct, not visit_type_FOO. Since it is this patch that is tweaking visit_type_FOO, I have to explain why visit_type_ENUM preserves values, while visit_type_OBJ sets to NULL. >> * >> - * FIXME: At present, input visitors may allocate an incomplete *@obj= >> - * even when visit_type_FOO() reports an error. Using an output >> - * visitor with an incomplete object has undefined behavior; callers >> - * must call qapi_free_FOO() (which uses the dealloc visitor, and >> - * safely handles an incomplete object) to avoid a memory leak. >> + * If an error is detected during visit_type_FOO() with an input >> + * visitor, then *@obj will be NULL for pointer types, and left >> + * unchanged for scalar types. >=20 > Okay. And this matches the commit message explaining the difference between scalar and object (and also applies to visit_type_int being a scalar that leaves the value unchanged on error). >=20 >> + * Using an output visitor with an >> + * incomplete object has undefined behavior (other than a special cas= e >> + * for visit_type_str() treating NULL like ""), while the dealloc >> + * visitor safely handles incomplete objects. >=20 > Where do the incomplete objects come from now? I thought this patch > gets rid of them. Still possible to create one by manual means, just no longer possible from a QAPI input visitor. I'll tweak the wording. >> -void visit_end_struct(Visitor *v); >> +bool visit_end_struct(Visitor *v); >=20 > I generally like functions to return something useful, but not in this > case, because the function name gives you no clue about its value. > Consider: >=20 > if (visit_end_struct(v) && err) { > qapi_free_FOO(*obj); > *obj =3D NULL; > } >=20 > To find out what this means, a reader not familiar with visitors almost= > certainly needs to refer to visit_end_struct()'s contract or code. >=20 > Compare: >=20 > visit_end_struct(v); > if (err && v->type =3D=3D VISITOR_INPUT) { v->type is a layering violation... > qapi_free_FOO(*obj); > *obj =3D NULL; > } >=20 > Or: >=20 > visit_end_struct(v); > if (err && visit_is_input(v)) { =2E..but this is doable by exporting visit_is_input(). > qapi_free_FOO(*obj); > *obj =3D NULL; > } Makes the generated code have more lines, but who really cares. So consider it done in v15. >> +++ b/qapi/qapi-visit-core.c >> @@ -23,11 +23,17 @@ >> void visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp) >> { >> + Error *err =3D NULL; >> + >> if (obj) { >> assert(size); >> assert(v->type !=3D VISITOR_OUTPUT || *obj); >> } >> - v->start_struct(v, name, obj, size, errp); >> + v->start_struct(v, name, obj, size, &err); >> + if (obj && v->type =3D=3D VISITOR_INPUT) { >> + assert(err || *obj); >> + } >> + error_propagate(errp, err); >=20 > Sure this belongs to this patch? More of the same below. Hmm. Patch 3 was the one that tightened semantics on visit_start, so I can certainly try to hoist the added assertions there. >> static void test_validate_fail_alternate(TestInputVisitorData *data, >> const void *unused) >> { >> - UserDefAlternate *tmp =3D NULL; >> + UserDefAlternate *tmp; >=20 > Did this initialization become dead in PATCH 03 already? Probably :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --l3cjSxVUMLE3lnbHXUU2mg8lg8hbTh8W1 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/ iQEcBAEBCAAGBQJXITRrAAoJEKeha0olJ0NqS0gH/1/m37jEbELLGEI/9zORm9qU 7rtncYm/EXUeEW7+R7N8c2Nb1SRHPUORomYLlj3SMPfNt7jWUF0rlk5GSV1l73ZI 5abmSg9NzgJ1oSXdUFIIoF7o2zl87fngDuzReYvZRiRyWBr7GA7BP7RbMSbROhep blaT7Bs1Scdjv544UWKIvUuHxjmqp8Mf1ry8PyQDC2Ua0TmKnwUY2Ev3o+EgzC/J OEQYSTu+Hbhcv4147KCUriQCVONGdP2ZnvFBrBD2/8SHVs8aCGjygoRfyHyNCkNk bJIxqip20QDhLWTrX3SQK0epoTmpReNkBwmo4oOmmMIhGgOwFtUVS8ewxxmkTxM= =Rn7p -----END PGP SIGNATURE----- --l3cjSxVUMLE3lnbHXUU2mg8lg8hbTh8W1--