Re: [Qemu-devel] [PATCH] hw/arm/virt: error_report cleanups

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 9 November 2015 at 10:21, Markus Armbruster <address@hidden> wrote:
>> Peter Maydell <address@hidden> writes:
>>
>>> On 9 November 2015 at 07:44, Markus Armbruster <address@hidden> 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.