From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45200) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biPSv-00074V-QY for qemu-devel@nongnu.org; Fri, 09 Sep 2016 13:19:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biPSr-0002s3-UK for qemu-devel@nongnu.org; Fri, 09 Sep 2016 13:19:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57838) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biPSr-0002rs-MP for qemu-devel@nongnu.org; Fri, 09 Sep 2016 13:19:45 -0400 Date: Fri, 9 Sep 2016 18:19:41 +0100 From: "Daniel P. Berrange" Message-ID: <20160909171941.GM25802@redhat.com> Reply-To: "Daniel P. Berrange" References: <1473228390-18669-1-git-send-email-peterx@redhat.com> <87inu5m2bj.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87inu5m2bj.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v4 0/4] Introduce error_report_{fatal|abort} List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Peter Xu , peter.maydell@linaro.org, famz@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com On Fri, Sep 09, 2016 at 07:05:04PM +0200, Markus Armbruster wrote: > Peter Xu writes: > > > v4 changes: > > - remove two standard headers since they are included in osdep.h > > already [Fam] > > - make sure it passes build on all platforms (no --target-list > > specified during configure) > > > > v3 changes: > > - implement error_report_fatal using function [Markus] > > - provide error_report_abort as well in seperate patch [Markus, Fam] > > > > We have many use cases that first print some error messages, then > > quit (by either exit() or abort()). This series introduce two helper > > functions for that. > > > > The old formats are mostly one of the following: > > > > Case one: > > > > error_report(...); > > exit(1|EXIT_FAILURE) | abort(); > > > > Case two: > > > > error_setg(&error_{fatal|abort}, ...); > > > > And we can convert either of the above cases into: > > > > error_report_{fatal|abort}(...); > > > > Two coccinelle scripts are created to help automate the work, plus > > some manual tweaks: > > > > 1. very long strings, fix for over-80-chars issues, to make sure it > > passes checkpatch.pl. > > > > 2. add "return XXX" for some non-void retcode functions. > > > > The first two patches introduce the functions. The latter two apply > > them. > > You effectively propose to revise this coding rule from error.h: > > * Please don't error_setg(&error_fatal, ...), use error_report() and > * exit(), because that's more obvious. > * Likewise, don't error_setg(&error_abort, ...), use assert(). > > If we accept your proposal, you get to add a patch to update the rule :) > > We've discussed the preferred way to report fatal errors to the human > user before. With actual patches, we can see how a change of rules > changes the code. Do we like the change shown by this patch set? > > I believe there are a number of separate issues to discuss here: > > * Shall we get rid of error_setg(&error_fatal, ...)? > > This is a no-brainer for me. Such a simple thing should be done in > one way, not two ways. I count 14 instances of > error_setg(&error_fatal, ...), but more than 300 of error_report(...); > exit(1). NB, arguably 99% of the usage of error_setg(&error_fatal) are in fact cases where code ought to be eventually converted to accept an "Error **errp" parameter and let the caller decide whether to exit or not. IOW, if we take this approach today we'll change error_setg(&error_fatal, "...."); into error_report_fatal("...."); and then later potentially change it back again to error_setg(errp, "...."); This isn't the end of the world, but I'm just not convinced that the intermediate change to error_report_fatal() buys us anything positive overall. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|