From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1axYuB-0002mW-F0 for qemu-devel@nongnu.org; Tue, 03 May 2016 07:54:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1axYty-0004GZ-TN for qemu-devel@nongnu.org; Tue, 03 May 2016 07:54:13 -0400 From: Markus Armbruster 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> <5727AB18.2010809@redhat.com> Date: Tue, 03 May 2016 13:53:18 +0200 In-Reply-To: <5727AB18.2010809@redhat.com> (Eric Blake's message of "Mon, 2 May 2016 13:31:36 -0600") Message-ID: <87h9ef8ij5.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: Kevin Wolf , famz@redhat.com, Michael Roth , "open list:Block layer core" , "Michael S. Tsirkin" , Juan Quintela , qemu-devel@nongnu.org, Alexander Graf , "open list:sPAPR" , Amit Shah , Max Reitz , Andreas =?utf-8?Q?F=C3=A4rber?= , David Gibson Eric Blake writes: > On 05/02/2016 12:20 PM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> 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. >> >> 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. Let's do it early. >>> @@ -59,7 +59,7 @@ struct Visitor >>> GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size); >>> >>> /* Must be set */ >>> - void (*end_list)(Visitor *v); >>> + void (*end_list)(Visitor *v, void **list); >> >> 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. Fewer casts is good, but symmetry is also good. >>> >>> /* 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); >> >> 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). Okay, I can buy this one. >>> +++ b/qapi/qapi-clone-visitor.c >>> @@ -29,11 +29,8 @@ static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, >>> QapiCloneVisitor *qcv = 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 easier */ >>> + /* Nothing to allocate on the virtual walk */ >>> assert(qcv->depth); >>> - qcv->depth++; >>> return; >>> } >>> >> >> 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(), ... > >> >>> @@ -41,11 +38,13 @@ static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, >>> qcv->depth++; >>> } >>> >>> -static void qapi_clone_end(Visitor *v) >>> +static void qapi_clone_end(Visitor *v, void **obj) >>> { >>> QapiCloneVisitor *qcv = to_qcv(v); >>> assert(qcv->depth); >>> - qcv->depth--; >>> + if (obj) { >>> + qcv->depth--; >>> + } > > ...and this is the matching elision of the depth manipulations. Okay, I'm confused. Consider BlockdevRef, defined as { 'alternate': 'BlockdevRef', 'data': { 'definition': 'BlockdevOptions', 'reference': 'str' } } where BlockdevOptions is a (flat) union. Let's clone a BlockdevRef holding a str. Sequence of calls: qapi_BlockdevRef_clone(src) qapi_clone_visitor_new(src) // qcv->depth is now 0 visit_type_BlockdevRef(v, NULL, &dst, &error_abort) visit_start_alternate(v, NULL, &dst, sizeof(BlockdevRef), true, &error_abort) qapi_clone_start_alternate(v, NULL, &dst, sizeof(BlockdevRef), true, &error_abort) qapi_clone_start_struct(v, NULL, &dst, sizeof(BlockdevRef), &error_abort) // does not increment qcv->depth visit_type_str(v, NULL, &dst->u.references, &error_abort) qapi_clone_type_str(v, NULL, &dst->u.references, &error_abort) assert(qcv->depth) // why does this hold? >>> +++ b/qapi/qmp-input-visitor.c >>> @@ -145,7 +145,7 @@ static void qmp_input_check_struct(Visitor *v, Error **errp) >>> } >>> } >>> >>> -static void qmp_input_pop(Visitor *v) >>> +static void qmp_input_pop(Visitor *v, void **obj) >>> { >>> QmpInputVisitor *qiv = to_qiv(v); >>> StackObject *tos = &qiv->stack[qiv->nb_stack - 1]; >> >> 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.