From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvrY4-0000g6-Kj for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:52:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvrY3-0001jL-LY for qemu-devel@nongnu.org; Mon, 09 Nov 2015 13:52:12 -0500 From: Markus Armbruster References: <1446909925-12201-1-git-send-email-drjones@redhat.com> <87twovpqg1.fsf@blackfin.pond.sub.org> <87oaf3jww7.fsf@blackfin.pond.sub.org> Date: Mon, 09 Nov 2015 19:52:03 +0100 In-Reply-To: (Peter Maydell's message of "Mon, 9 Nov 2015 15:47:00 +0000") Message-ID: <878u67atv0.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Andrew Jones , qemu-arm@nongnu.org, QEMU Developers Peter Maydell writes: > On 9 November 2015 at 10:21, Markus Armbruster wrote: >> Peter Maydell writes: >> >>> On 9 November 2015 at 07:44, Markus Armbruster wrote: >>>> For consistency, error messages should be a phrase, not a full sentence, >>>> let alone a paraphraph. >>> >>> This is in direct conflict with wanting them to be actually useful >>> to users :-( >> >> I appreciate your drive for useful error messages. Judging from the >> error messages we got, it's a rare thing. >> >> Let me rephrase. The error message proper (the thing emitted by >> error_report()) should be a phrase, and it should be short and to the >> point. It can be followed by hints. Compare: >> >> qemu-system-arm: Unable to determine GIC version supported by >> host. KVM acceleration is probably not supported. >> >> and >> >> qemu-system-arm: Unable to determine GIC version supported by host >> KVM acceleration is probably not supported >> >> I prefer the latter. The error message proper is short and to the >> point. The hint points to the most probable cause. Sensible line >> lengths. > > I agree that the latter is preferable; I had been under the > impression that we weren't allowed to use newlines in error > messages, though... error_report()'s contract: /* * Print an error message to current monitor if we have one, else to stderr. * Format arguments like sprintf(). The result should not contain * newlines. * Prepend the current location and append a newline. * It's wrong to call this in a QMP monitor. Use error_setg() there. */ If you want to print additional information, that's totally fine! Use error-printf(). >> By the way, the error.h API supports this message + hints convention >> since commit 50b7b00. > > Thanks, I had missed this useful improvement to the API. > How does it work in cases like this where we don't have > an Error* to fill in? You do what error_report_err() would do had you had an Error *err to fill in: void error_report_err(Error *err) { error_report("%s", error_get_pretty(err)); if (err->hint) { error_printf_unless_qmp("%s\n", err->hint->str); } error_free(err); } In other words, you print the error message proper with error_report(), and the additional information with error_printf(). error_report_err() uses error_printf_unless_qmp() instead because it wants to tolerate misuse in QMP context. It isn't an assertion failure error mostly for historical reasons.