From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOVdx-0007A0-MA for qemu-devel@nongnu.org; Wed, 27 Jan 2016 14:20:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOVdt-0005t3-KG for qemu-devel@nongnu.org; Wed, 27 Jan 2016 14:20:41 -0500 Received: from roura.ac.upc.es ([147.83.33.10]:59535) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOVdt-0005sk-7p for qemu-devel@nongnu.org; Wed, 27 Jan 2016 14:20:37 -0500 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <145286604004.29455.1509463776346981362.stgit@localhost> <145286604762.29455.8630766735054984295.stgit@localhost> <569D4A82.5000506@redhat.com> <569E0422.8040806@redhat.com> <8760yo735q.fsf@fimbulvetr.bsc.es> <56A8B211.9000803@redhat.com> <87twlyj10b.fsf@fimbulvetr.bsc.es> Date: Wed, 27 Jan 2016 20:20:32 +0100 In-Reply-To: <87twlyj10b.fsf@fimbulvetr.bsc.es> (=?utf-8?Q?=22Llu=C3=ADs?= Vilanova"'s message of "Wed, 27 Jan 2016 20:06:12 +0100") Message-ID: <87egd2j0cf.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , "Dr . David Alan Gilbert" Llu=C3=ADs Vilanova writes: > Thomas Huth writes: >> On 20.01.2016 15:10, Llu=C3=ADs Vilanova wrote: >>> Thomas Huth writes: >>>=20 >>>> On 18.01.2016 21:26, Eric Blake wrote: >>>>> On 01/15/2016 06:54 AM, Llu=C3=ADs Vilanova wrote: >>>>>> Gives some general guidelines for reporting errors in QEMU. >>>>>>=20 >>>>>> Signed-off-by: Llu=C3=ADs Vilanova >>>>>> --- >>>>>> HACKING | 36 ++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>> ... >>>>>> +Functions in this header are used to accumulate error messages in a= n 'Error' >>>>>> +object, which can be propagated up the call chain where it is final= ly reported. >>>>>> + >>>>>> +In its simplest form, you can immediately report an error with: >>>>>> + >>>>>> + error_setg(&error_fatal, "Error with %s", "arguments"); >>>>>=20 >>>>> This paradigm doesn't appear anywhere in the current code base >>>>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but >>>>> nothing directly passes error_fatal). It's a bit odd to document >>>>> something that isn't actually used. >>>=20 >>>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want >>>> something like this, I think we should introduce a proper >>>> error_report_fatal() function instead. >>>=20 >>> That's a bit of a chicken and egg problem. My main intention was to pro= vide a >>> best practices summary on reporting messages/errors, since QEMU's code = is really >>> heterogeneous on that regard. But there seems to be no consensus on som= e details >>> of what the proper way should be with the current interfaces. >>>=20 >>> Utility functions for "regular messages", warnings, fatals and aborts w= ould >>> definitiely be an improvement IMHO, but I dont have time to adapt exist= ing code >>> to these (and I was told not to add unused utility functions for this). >>>=20 >>> Now, if I were able to add such functions, it'd be something like: >>>=20 >>> // Generate message "as is"; not sure if this should exist. >>> message_report(fmt, ...) >> Not sure what this should be good for? We've already got error_report() >> that generates messages "as is", haven't we? > Well, it just seemed wrong to me using error_report() to report "regular > messages" :) >>> // Generate message with prepended file/line information for the caller. >>> // Calls exit/abort on the last two. >>> error_report_{warn,fatal,abort}(fmt, ...) >>>=20 >>> // Same with an added message from strerror. >>> error_report_{warn,fatal,abort}_errno(fmt, ...) >>>=20 >>> But, should I add these without providing extensive patches that refact= or code >>> to use them? >> Maybe create a patch that introduces the _fatal and _abort functions >> (I'd skip the _warn functions for now), and use them in one or two files >> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should >> not be that much of work, and could be a good base for further discussio= n? > I can do that. But then should 'error_fatal' and 'error_abort' be officia= lly > deprecated in favour of error_report_fatal() and error_report_abort()? Sorry, I see this is misleading. I mean deprecate directly using "error_setg(error_fatal)"; you can still decide to pass error_fatal as an e= rror object to other user functions. Cheers, Lluis