Peter Maydell <peter.mayd...@linaro.org> writes:
> On 9 November 2015 at 10:21, Markus Armbruster <arm...@redhat.com> wrote:
>> Peter Maydell <peter.mayd...@linaro.org> writes:
>>
>>> On 9 November 2015 at 07:44, Markus Armbruster <arm...@redhat.com> 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.
