From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZL4yP-0008Ib-TW for qemu-devel@nongnu.org; Fri, 31 Jul 2015 03:43:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZL4yK-0002ge-Uq for qemu-devel@nongnu.org; Fri, 31 Jul 2015 03:43:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58747) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZL4yK-0002gA-P2 for qemu-devel@nongnu.org; Fri, 31 Jul 2015 03:43:16 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-30-git-send-email-armbru@redhat.com> <55B025B9.2060004@redhat.com> <87zj2gohv1.fsf@blackfin.pond.sub.org> <55B79754.6090609@redhat.com> <87y4hzicto.fsf@blackfin.pond.sub.org> <55B8F40E.9020301@redhat.com> <877fpic20s.fsf@blackfin.pond.sub.org> <55BA3270.90309@redhat.com> <87lhdxy6wy.fsf@blackfin.pond.sub.org> <55BAA9D2.6020205@redhat.com> Date: Fri, 31 Jul 2015 09:43:12 +0200 In-Reply-To: <55BAA9D2.6020205@redhat.com> (Eric Blake's message of "Thu, 30 Jul 2015 16:48:50 -0600") Message-ID: <874mkkvkkv.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake writes: > On 07/30/2015 09:57 AM, Markus Armbruster wrote: >>> For qmp_FOO(), this is a reasonable contract. But our very own >>> generated code does not follow these rules: visit_type_FOO() can assign >>> into *obj even when setting an error, if it encounters a parse error >>> halfway through the struct, leaving the caller responsible to still >>> clean up the mess if it wants to avoid a memory leak. >> >> Assigning to *obj, then fail is tolerable[*]. Relying on the caller to >> free it is not. If we do that, it's a bug. >> >>> Maybe that means our generated code needs to be reworked to properly >>> clean up on a failed parse, such that *obj is guaranteed to be NULL if >>> an error is returned. As a separate patch, of course. >> >> Yes. Would you like to propose a FIXME for me to put into this series? > > Done (see 12.5/47). > >> >> >> [*] Leaving *obj alone on failure is nicer, but may not always be >> practical. > > I'm wondering if visit_end_struct() should be changed to accept a bool > parameter that is true if the struct is ended normally, and false if an > error has been detected. We may also need to alter visit_start_struct > to return a bool (true if an object was allocated), to help feed our > visitor logic on whether to pass true or false to the visit_end_struct(). > > We did something like that for visit_start_union()/visit_end_union(), > but I suspect those visitor interfaces are also a bit screwy. Oh well, > I'm not going to try and tweak it today, but am happy with just adding > the FIXME. We need to keep the introspection series reasonably focused. I can't afford a detour into visitor semantics right now, so we'll go with your FIXMEs. > Also, it would be really nice if we had docs in visitor.h and/or > visitor-impl.h, to get some sort of feel for how the visitor is supposed > to be used. Indeed. We got a grand total of three one-liner comments, and one of them is inaccurate: actual visitors don't obey /* Must be set */.