From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51614) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKMml-0000FF-Nh for qemu-devel@nongnu.org; Wed, 29 Jul 2015 04:32:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKMmk-0005B4-EY for qemu-devel@nongnu.org; Wed, 29 Jul 2015 04:32:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKMmk-0005Aw-7V for qemu-devel@nongnu.org; Wed, 29 Jul 2015 04:32:22 -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> Date: Wed, 29 Jul 2015 10:32:19 +0200 In-Reply-To: <55B79754.6090609@redhat.com> (Eric Blake's message of "Tue, 28 Jul 2015 08:53:08 -0600") Message-ID: <87y4hzicto.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/28/2015 01:34 AM, Markus Armbruster wrote: >> Let me rephrase to make sure I understand. >> >> Ignore the (not rets) case, because retval doesn't exist then. >> >> qmp_marshal_output_FOO() visits retval twice. First, with a QMP output >> visitor to do the actual marshalling. Second, with a QAPI dealloc >> visitor to destroy it. > > And the use of the dealloc visitor is buried inside the > qmp_marshal_output_FOO() call. > >> >> If we execute the assignment to retval, we must go on to call >> qmp_marshal_output_FOO(), or else we have a leak. >> >> If we can reach qmp_marshal_output_FOO() without executing the >> assignment, we must initialize retval. If we can't, any initialization >> is unused. >> >> gen_call() generates code of the form >> >> retval = qmp_FOO(... args ..., &local_err); >> if (local_err) { >> goto out; >> } >> >> qmp_marshal_output_FOO(retval, ret, &local_err); >> > >> Observe: >> >> 1. The assignment dominates the only use. Therefore, the initialization >> is unused. Let's drop it in a separate cleanup patch. >> >> 2. We can leak retval only when qmp_FOO() returns non-null and local_err >> is non-null. This must not happen, because: >> >> a. local_err must be null before the call, and >> >> b. the call must not return non-null when it sets local_err. > > We don't state that contract anywhere, but I doubt any of the qmp_FOO() > functions violate it, so it is worth making it part of the contract. It's a general Error API rule: set an error exactly on failure. It applies to any function returning errors through an Error **errp parameter, and we generally don't bother to spell it out for the individual functions. The part that needs to be spelling out is what success and failure mean. A qmp_FOO() returning an object returns null on failure. >> We could right after out: assert(!local_err || !retval). Not sure >> it's worthwhile. > > I think it IS worthwhile, because it would catch buggy callers. Not We use the same assumption all over the place, without asserting it. The boilerplate around calls using the Error API is bad enough as it is. I'm reluctant to make it worse by adding assertions. Less of an issue in generated code, but violations are even less likely there. However, I'm reluctant to assert in some places, but not all, because inconsistency breeds confusion. In general, I prefer a function's pre- and post-conditions to be asserted in the function itself, not in its callers. Asserting selected parts of the post-condition in callers can be occasionally helpful to help human readers and static analyzers lacking insight into the function. > sure if after out: is the right place (then you'd need an initializer to > cover any other code that jumps to out), but this would do the same > > retval = qmp_FOO(...); > if (local_err) { > assert(!retval); > goto out; > } > qmp_marshal_output_FOO(retval, ...); Not quite the same: it doesn't catch the case !local_err && !retval. But it doesn't matter, because that case is bound to die in qmp_marshal_output_FOO(). >> TL;DR: I concur with your analysis. > > Is it worth dropping the dead initializer and adding the assert in the > same pre-req cleanup patch? Do you want me to submit it since I did the > analysis? Let's drop the useless initializer. As explained above, I don't like the assertion for reasons explained above, but if you want it, I'm willing to take it anyway, in a separate follow-up patch. I'd prefer to drop the initializer myself myself (with you credited in the commit message), because it's certainly less total work, and quite possibly less work for just for me.