From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40516) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOOp3-0005Ov-Op for qemu-devel@nongnu.org; Wed, 27 Jan 2016 07:03:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOOox-0002td-Rs for qemu-devel@nongnu.org; Wed, 27 Jan 2016 07:03:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56084) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOOox-0002tP-KO for qemu-devel@nongnu.org; Wed, 27 Jan 2016 07:03:35 -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> From: Thomas Huth Message-ID: <56A8B211.9000803@redhat.com> Date: Wed, 27 Jan 2016 13:03:29 +0100 MIME-Version: 1.0 In-Reply-To: <8760yo735q.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 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. >>>> >>>> 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"); >>> >>> 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? > // Generate message with prepended file/line information for the call= er. > // 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 discussion= ? Thomas