From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57496) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOnXZ-0001vT-1z for qemu-devel@nongnu.org; Thu, 28 Jan 2016 09:27:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOnXU-0001nl-Bi for qemu-devel@nongnu.org; Thu, 28 Jan 2016 09:27:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOnXU-0001nD-4z for qemu-devel@nongnu.org; Thu, 28 Jan 2016 09:27:12 -0500 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> <87egd2j0cf.fsf@fimbulvetr.bsc.es> From: Thomas Huth Message-ID: <56AA253A.2050503@redhat.com> Date: Thu, 28 Jan 2016 15:27:06 +0100 MIME-Version: 1.0 In-Reply-To: <87egd2j0cf.fsf@fimbulvetr.bsc.es> 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: =?UTF-8?Q?Llu=c3=ads_Vilanova?= Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , "Dr . David Alan Gilbert" On 27.01.2016 20:20, Llu=C3=ADs Vilanova wrote: > Llu=C3=ADs Vilanova writes: >=20 >> Thomas Huth writes: >>> On 20.01.2016 15:10, Llu=C3=ADs Vilanova wrote: >>>> Thomas Huth writes: >>>> >>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Llu=C3=ADs Vilanova >>>>>>> --- >>>>>>> HACKING | 36 ++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 36 insertions(+) >>>>> ... >>>>>>> +Functions in this header are used to accumulate error messages i= n an 'Error' >>>>>>> +object, which can be propagated up the call chain where it is fi= nally reported. >>>>>>> + >>>>>>> +In its simplest form, you can immediately report an error with: >>>>>>> + >>>>>>> + error_setg(&error_fatal, "Error with %s", "arguments"); >>>>>> >>>>>> 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. >>>> >>>>> +1 for _not_ documenting this here: IMHO this looks ugly. If we wan= t >>>>> something like this, I think we should introduce a proper >>>>> error_report_fatal() function instead. >>>> >>>> That's a bit of a chicken and egg problem. My main intention was to = provide a >>>> best practices summary on reporting messages/errors, since QEMU's co= de is really >>>> heterogeneous on that regard. But there seems to be no consensus on = some details >>>> of what the proper way should be with the current interfaces. >>>> >>>> Utility functions for "regular messages", warnings, fatals and abort= s would >>>> definitiely be an improvement IMHO, but I dont have time to adapt ex= isting code >>>> to these (and I was told not to add unused utility functions for thi= s). >>>> >>>> Now, if I were able to add such functions, it'd be something like: >>>> >>>> // Generate message "as is"; not sure if this should exist. >>>> message_report(fmt, ...) >=20 >>> Not sure what this should be good for? We've already got error_report= () >>> that generates messages "as is", haven't we? >=20 >> Well, it just seemed wrong to me using error_report() to report "regul= ar >> messages" :) >=20 >=20 >>>> // Generate message with prepended file/line information for the cal= ler. >>>> // Calls exit/abort on the last two. >>>> error_report_{warn,fatal,abort}(fmt, ...) >>>> >>>> // Same with an added message from strerror. >>>> error_report_{warn,fatal,abort}_errno(fmt, ...) >>>> >>>> But, should I add these without providing extensive patches that ref= actor code >>>> to use them? >=20 >>> 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 fi= les >>> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That sho= uld >>> not be that much of work, and could be a good base for further discus= sion? >=20 >> I can do that. But then should 'error_fatal' and 'error_abort' be offi= cially >> deprecated in favour of error_report_fatal() and error_report_abort()? >=20 > 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 error > object to other user functions. Since we hardly got any of these in the code right now, I don't see an urgent need to explicitely say that this should be deprecated. I hope that people rather will use the new functions automatically instead since these sounds much more intuitive, IMHO. Thomas