From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vnit2-0008TG-Nn for qemu-devel@nongnu.org; Tue, 03 Dec 2013 00:51:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vnisv-0001mb-Qt for qemu-devel@nongnu.org; Tue, 03 Dec 2013 00:51:08 -0500 Received: from mail-wg0-f51.google.com ([74.125.82.51]:33016) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vnisv-0001mX-L0 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 00:51:01 -0500 Received: by mail-wg0-f51.google.com with SMTP id b13so9977329wgh.6 for ; Mon, 02 Dec 2013 21:51:00 -0800 (PST) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <87li071w02.fsf@blackfin.pond.sub.org> References: <1385601858-8065-1-git-send-email-imammedo@redhat.com> <20131128142323.01a88c7e@nial.usersys.redhat.com> <5297480B.9050501@redhat.com> <87fvqgk1q6.fsf@blackfin.pond.sub.org> <87li071w02.fsf@blackfin.pond.sub.org> Date: Tue, 3 Dec 2013 15:51:00 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Michael S. Tsirkin" , "qemu-devel@nongnu.org Developers" , Anthony Liguori , Paolo Bonzini , Igor Mammedov , =?ISO-8859-1?Q?Andreas_F=E4rber?= On Fri, Nov 29, 2013 at 5:56 PM, Markus Armbruster wrote: > Peter Crosthwaite writes: > >> On Fri, Nov 29, 2013 at 1:03 AM, Markus Armbruster wrote: >>> Paolo Bonzini writes: >>> >>>> Il 28/11/2013 14:23, Igor Mammedov ha scritto: >>>>> > object_property_set(Foo, bar, "baz", &abort_on_err); >>>>> >>>>> that is just another way to put burden on caller, instead of doing it >>>>> in one place. >>>> >>>> It's also much more self-documenting. >>>> >>>> The problem is that there is one very good case where you want the >>>> silent-don't-care behavior: when you don't care about the exact error, >>>> and you can figure out whether there was an error from the returned >>>> value of the function. This doesn't apply to object_property_set of >>>> course, but it is the reason why NULL Error* has silent-don't-care behavior. >>>> >>>> Now, let's look at the alternatives: >>>> >>>> * keep silent don't care >>>> + consistent >>>> + predictable >>>> - not always handy >>>> >>>> * only modify object_property_set >>>> + mostly does the right thing >>>> - inconsistent with other Error* functions >>>> - inconsistent with _nofail functions >>>> >>>> * Peter's alternative >>>> + self-documenting >>>> + consistent >>>> + predictable >>>> >>>> * make Error* mandatory for all void functions >>>> + consistent >>>> + almost predictable (because in C you can ignore return values) >>>> - not necessarily does the right thing (e.g. cleanup functions) >>>> - requires manual effort to abide to the policy >>>> >>>> I vote for Peter's proposal, or for adding object_property_set_nofail. >>>> No particular preference. >>>> >>>> Another variant: modify object_property_set to add a g_warning. I think >>>> it's fine. It reduces the inconsistency, and still simplifies debugging. >>> >>> I like Peter's proposal, provided we use it to get rid of the _nofail >>> pattern. >>> >>> Second preference is adding another _nofail wrapper. >>> >> >> So this issue with _nofail is that if you are doing it properly, every >> API needed both normal and _nofail version of every function. APIs >> generally have no bussiness dictating failure policy so by extension, >> normal and _nofail should exist for every API that accepts an Error *. >> With my proposal, its fixed once, in one place and we can torch all >> the _nofail boilerplate all over the tree as you have suggested. >> >> These is another subtle advantage to my proposal, and that is that >> assertions can happen at the point of failure in the offending API, >> not after the fact in the caller. If the caller does an >> assert_no_error, then the abort() happens after return from the API >> call. This makes debugging awkward cause the stack frames into the API >> call where the issue actually occured are lost. Whereas if error_set >> does the abort() you will get all stack frames into the API call where >> the issue occured when gdb traps the abort(). > > To make further progress, we need a patch. Care to cook one up? > Done. Regards, Peter