From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38214) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKV3e-0004Lc-S1 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 13:22:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKV3W-0006Kd-Hi for qemu-devel@nongnu.org; Wed, 29 Jul 2015 13:22:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47551) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKV3W-0006KX-CJ for qemu-devel@nongnu.org; Wed, 29 Jul 2015 13:22:14 -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> Date: Wed, 29 Jul 2015 19:22:11 +0200 In-Reply-To: <55B8F40E.9020301@redhat.com> (Eric Blake's message of "Wed, 29 Jul 2015 09:41:02 -0600") Message-ID: <877fpic20s.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/29/2015 02:32 AM, Markus Armbruster wrote: > >>>> 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. > > Okay, you've got a point there. > >> >> 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. > > No need to add assertions. Maybe worth a patch to add a comment > somewhere, maybe as a new section in docs/qapi-code-gen.txt, documenting > how to write a handler and hook it into qmp-commands.hx, and what > conditions the handler must obey. And I'm fine with you dropping the > initializer as part of your v3 series. Sounds like a plan. Thanks!