From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axJaD-0004l3-7f for qemu-devel@nongnu.org; Mon, 02 May 2016 15:32:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axJZs-0003JO-5a for qemu-devel@nongnu.org; Mon, 02 May 2016 15:32:35 -0400 References: <1461903820-3092-1-git-send-email-eblake@redhat.com> <1461903820-3092-19-git-send-email-eblake@redhat.com> <87y47se2yy.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <5727AB18.2010809@redhat.com> Date: Mon, 2 May 2016 13:31:36 -0600 MIME-Version: 1.0 In-Reply-To: <87y47se2yy.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gdLutHEn9tR2bP0M0du0UjS1tXngPQAfX" Subject: Re: [Qemu-devel] [PATCH v3 18/18] qapi: Add parameter to visit_end_* List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Kevin Wolf , Alexander Graf , famz@redhat.com, "open list:Block layer core" , "Michael S. Tsirkin" , Juan Quintela , Michael Roth , "open list:sPAPR" , Amit Shah , Max Reitz , =?UTF-8?Q?Andreas_F=c3=a4rber?= , David Gibson This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gdLutHEn9tR2bP0M0du0UjS1tXngPQAfX Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/02/2016 12:20 PM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Rather than making the dealloc visitor track of stack of pointers >> remembered during visit_start_* in order to free them during >> visit_end_*, it's a lot easier to just make all callers pass the >> same pointer to visit_end_*. The generated code has access to the >> same pointer, while all other users are doing virtual walks and >> can pass NULL. The dealloc visitor is then greatly simplified. >> >> The clone visitor also gets a minor simplification of not having >> to track quite as much depth. >=20 > Do this before adding the clone visitor, to reduce churn? I could. I first thought of it after, and kept it there as a justification for keeping it (multiple visitors benefit), but even if rebased earlier, the commit message can still mention that it will make the clone visitor easier. >> @@ -59,7 +59,7 @@ struct Visitor >> GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t s= ize); >> >> /* Must be set */ >> - void (*end_list)(Visitor *v); >> + void (*end_list)(Visitor *v, void **list); >=20 > Sure you want void ** and not GenericList **? Yes. There are only two callers: dtc code passes NULL (where the type doesn't matter), and generated visit_type_FOOList() has to already cast to GenericList() during visit_start_list(); accepting void** here instead of GenericList** removes the need for a second instance of that cast. >=20 >> >> /* Must be set by input and dealloc visitors to visit alternates;= >> * optional for output visitors. */ >> @@ -68,7 +68,7 @@ struct Visitor >> bool promote_int, Error **errp); >> >> /* Optional, needed for dealloc visitor */ >> - void (*end_alternate)(Visitor *v); >> + void (*end_alternate)(Visitor *v, void **obj); >=20 > Sure you want void ** and not GenericAlternate **? Only one caller: generated code. Same story that we already have to cast during visit_start_alternate(), so using void** here avoids the need for a second cast. Oh, and the clone visitor was easier to write with a single function that takes void** for all three visit_end() implementations (whereas I'd have to write three functions identical except for signature otherwise). >> +++ b/qapi/qapi-clone-visitor.c >> @@ -29,11 +29,8 @@ static void qapi_clone_start_struct(Visitor *v, con= st char *name, void **obj, >> QapiCloneVisitor *qcv =3D to_qcv(v); >> >> if (!obj) { >> - /* Nothing to allocate on the virtual walk during an >> - * alternate, but we still have to push depth. >> - * FIXME: passing obj to visit_end_struct would make this eas= ier */ >> + /* Nothing to allocate on the virtual walk */ >> assert(qcv->depth); >> - qcv->depth++; >> return; >> } >> >=20 > Why can we elide qcv->depth++? Do the assert(qcv->qdepth) still hold? Yes, the assertion still holds: we can only reach this code underneath visit_start_alternate(), ... >=20 >> @@ -41,11 +38,13 @@ static void qapi_clone_start_struct(Visitor *v, co= nst char *name, void **obj, >> qcv->depth++; >> } >> >> -static void qapi_clone_end(Visitor *v) >> +static void qapi_clone_end(Visitor *v, void **obj) >> { >> QapiCloneVisitor *qcv =3D to_qcv(v); >> assert(qcv->depth); >> - qcv->depth--; >> + if (obj) { >> + qcv->depth--; >> + } =2E..and this is the matching elision of the depth manipulations. >> +++ b/qapi/qmp-input-visitor.c >> @@ -145,7 +145,7 @@ static void qmp_input_check_struct(Visitor *v, Err= or **errp) >> } >> } >> >> -static void qmp_input_pop(Visitor *v) >> +static void qmp_input_pop(Visitor *v, void **obj) >> { >> QmpInputVisitor *qiv =3D to_qiv(v); >> StackObject *tos =3D &qiv->stack[qiv->nb_stack - 1]; >=20 > You could assert @obj matches tos->obj. Same for the other visitors > that still need a stack. And brings me back to my question of whether qapi-visit-core.c should maintain its own stack for asserting these types of sanity checks for ALL callers, even when the visitor itself doesn't need a stack. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --gdLutHEn9tR2bP0M0du0UjS1tXngPQAfX 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/ iQEcBAEBCAAGBQJXJ6sYAAoJEKeha0olJ0NqEdYH/RMsK69OaX49WvOEiK51yQMz tftI83QXy1GDaz0ckNyRw/J74fplt5NHO9fXF8/XUtnEYLr3mOjng491kSdhyN4V RuWMZCg0t4bdnjDmhfOdq9z4M7FOta1JOzDY0OyOLqcCWkq4NmoODxeHcoLCypJF 6yGovhICRCV+EmTmC7VeeRATd63JP15XAD/xj6qHsPPY3a2z60lSk8N9MhFTXRrt rYaQ5EQ1GWiATARqAZufBsewzfJLbkDG+EewRh4SHHTuDMv5EmErj6WiasHIOzeA a8VXEPILGyTp7I9tUMT/+V1zXODIN0P33NHswCzb1w16h6HXjx475kD3tYXR2sU= =W4mo -----END PGP SIGNATURE----- --gdLutHEn9tR2bP0M0du0UjS1tXngPQAfX--