From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQg4o-0008JX-KM for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:53:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQg4k-0001rW-ER for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:53:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQg4k-0001r9-7b for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:53:18 -0500 From: Markus Armbruster References: <145442963048.1539.13602468921796488810.stgit@localhost> <145442963860.1539.7135815311391731257.stgit@localhost> Date: Tue, 02 Feb 2016 19:53:11 +0100 In-Reply-To: <145442963860.1539.7135815311391731257.stgit@localhost> (=?utf-8?Q?=22Llu=C3=ADs?= Vilanova"'s message of "Tue, 2 Feb 2016 17:13:58 +0100") Message-ID: <87twlraqqw.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 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: =?utf-8?Q?Llu=C3=ADs?= Vilanova Cc: Stefan Hajnoczi , Thomas Huth , David Gibson , qemu-devel@nongnu.org, "Dr . David Alan Gilbert" I'm struggling with my review queue, and have had to resort to subsystem batching to increase throughput. Because of that, v3-v5 have flown by without a peep from me. My sincere apologies. Llu=C3=ADs Vilanova writes: > Provide two lean functions to report error messages that fatal/abort > QEMU. > > Signed-off-by: Llu=C3=ADs Vilanova > --- > include/qemu/error-report.h | 19 +++++++++++++++++++ > util/qemu-error.c | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.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=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_FMT_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. PATCH 3 actually does this for a couple of cases. > +/* Report message with caller location and abort */ > +#define error_vreport_abort(fmt, ap) \ > + do { \ > + error_report_abort_caller_internal(__FILE__, __LINE__, __func__)= ; \ > + error_vreport_abort_internal(fmt, ap); \ > + } while (0) > +#define error_report_abort(fmt, ...) \ > + do { \ > + error_report_abort_caller_internal(__FILE__, __LINE__, __func__)= ; \ > + error_report_abort_internal(fmt, ##__VA_ARGS__); \ > + } while (0) > + > +void error_report_abort_caller_internal(const char *file, int line, cons= t char *func); > +void QEMU_NORETURN error_vreport_abort_internal(const char *fmt, va_list= ap) GCC_FMT_ATTR(1, 0); > +void QEMU_NORETURN error_report_abort_internal(const char *fmt, ...) GCC= _FMT_ATTR(1, 2); > + > #endif Feature not mentioned in the commit message: your new form prints an additional line with source location information, like &error_abort does. See error_report_abort_internal() below. PATCH 4 uses error_report_abort() to change a dozen abort() to error_report_abort(... some message ...); or even error_report_abort(" "); Two aspects: source code and behavior. Source code: I don't think it makes it more readable. Behavior: prints a message before it crashes. The messages look like they make sense only to developers, not to users. This isn't surprising; these are internal errors, and you generally can't explain internal errors without referring to internal concepts the user doesn't know about. Should they happen, you need to debug anyway. As to the " " messages: surely you're joking, Mr. Feynman :) I feel the message adds very little information to the core dump for developers, and none for users. If the error message is genuinely useful to users, chances are we should exit(1) rather than abort(). If the QEMU developer community should decide I'm wrong and we really want to decorate abort()s with messages, we'd have to decorate the majority of the 600+ we have so that people can see the pattern. Without that, new undecorated ones will pop up faster than we can decorate them. In other words, the dozen you converted to demonstrate the idea for your RFC are a placeholder for one of these tree-wide transitions that are easier to start than to finish. PATCH 2 uses error_report_abort() to clean up error_setg(&error_abort, ... some message ...); to error_report_abort(... some message ...); I agree these uses of &error_abort should be cleaned up. However, I'd clean them up to abort() or assert(), for the reasons I just explained, and because that's what we do elsewhere. > diff --git a/util/qemu-error.c b/util/qemu-error.c > index ecf5708..3de002b 100644 > --- a/util/qemu-error.c > +++ b/util/qemu-error.c > @@ -237,3 +237,36 @@ void error_report(const char *fmt, ...) > error_vreport(fmt, ap); > va_end(ap); > } > + > +void error_vreport_fatal(const char *fmt, va_list ap) > +{ > + error_vreport(fmt, ap); > + exit(1); > +} > + > +void error_report_fatal(const char *fmt, ...) > +{ > + va_list ap; > + va_start(ap, fmt); > + error_vreport_fatal(fmt, ap); > + va_end(ap); > +} > + > +void error_report_abort_caller_internal(const char *file, int line, cons= t char *func) > +{ > + error_report("Unexpected error in %s() at %s:%d:", func, file, line); Should use error_printf(), so we get Unexpected error in frobnicate() at frob.c:666: qemu-system-x86_64: --frobnicate: Out of frobs instead of qemu-system-x86_64: --frobnicate: Unexpected error in frobnicate() at f= rob.c:666: qemu-system-x86_64: --frobnicate: Out of frobs > +} > + > +void error_vreport_abort_internal(const char *fmt, va_list ap) > +{ > + error_vreport(fmt, ap); > + abort(); > +} > + > +void error_report_abort_internal(const char *fmt, ...) > +{ > + va_list ap; > + va_start(ap, fmt); > + error_vreport_abort_internal(fmt, ap); > + va_end(ap); > +}