From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQuDG-000328-Ii for qemu-devel@nongnu.org; Wed, 03 Feb 2016 04:59:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQuDC-0007Nu-1v for qemu-devel@nongnu.org; Wed, 03 Feb 2016 04:59:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55497) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQuDB-0007Np-RE for qemu-devel@nongnu.org; Wed, 03 Feb 2016 04:58:57 -0500 References: <145442963048.1539.13602468921796488810.stgit@localhost> <145442963860.1539.7135815311391731257.stgit@localhost> <87twlraqqw.fsf@blackfin.pond.sub.org> <56B123F7.50505@redhat.com> <20160203050436.GI15080@voom.fritz.box> <87bn7yxgxm.fsf@blackfin.pond.sub.org> From: Thomas Huth Message-ID: <56B1CF5C.7040007@redhat.com> Date: Wed, 3 Feb 2016 10:58:52 +0100 MIME-Version: 1.0 In-Reply-To: <87bn7yxgxm.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , =?UTF-8?Q?Llu=c3=ads_Vilanova?= Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , David Gibson On 03.02.2016 10:48, Markus Armbruster wrote: > David Gibson writes: >=20 >> On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote: >>> On 02.02.2016 19:53, Markus Armbruster wrote: >>>> Llu=C3=ADs Vilanova writes: >>> ... >>> >>>>> diff --git a/include/qemu/error-report.h b/include/qemu/error-repor= t.h >>>>> index 7ab2355..6c2f142 100644 >>>>> --- a/include/qemu/error-report.h >>>>> +++ b/include/qemu/error-report.h >>>>> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_= ATTR(1, 2); >>>>> const char *error_get_progname(void); >>>>> extern bool enable_timestamp_msg; >>>>> =20 >>>>> +/* Report message and exit with error */ >>>>> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap= ) GCC_FMT_ATTR(1, 0); >>>>> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FM= T_ATTR(1, 2); >>>> >>>> This lets people write things like >>>> >>>> error_report_fatal("The sky is falling"); >>>> >>>> instead of >>>> >>>> error_report("The sky is falling"); >>>> exit(1); >>>> >>>> or >>>> >>>> fprintf(stderr, "The sky is falling\n"); >>>> exit(1); >>>> >>>> I don't think that's an improvement in clarity. >>> >>> The problem is not the existing code, but that in a couple of new >>> patches, I've now already seen that people are trying to use >>> >>> error_setg(&error_fatal, ... ); >> >> So, I don't actually see any real advantage to error_report_fatal(...) >> over error_setg(&error_fatal, ...). >=20 > I do. Compare: >=20 > (a) error_report(...); > exit(1); >=20 > (b) error_report_fatal(...); >=20 > (c) error_setg(&error_fatal, ...); >=20 > In my opinion, (a) is clearest: even a relatively clueless reader will > know what exit(1) does, can guess what error_report() approximately > does, and doesn't need to know what it does exactly. (b) is slightly > less obvious, and (c) is positively opaque. >=20 > Let's stick to the obvious (a) and be done with it. Ok, (a) is fine for me too, as long as we avoid (c). Llu=C3=ADs, could yo= u maybe add that information to your patch that updates the HACKING text? (and sorry for the fuzz with error_report_fatal() ... I thought it would be a good solution to avoid (c), but if (a) is preferred instead, then we should go with that solution instead). And, by the way, what about the spots that currently already use error_setg(&error_abort, ....) ? Should they be turned into error_report() + abort() instead? Or only abort(), without error message, since abort() is only about programming errors? Thomas