Re: [Qemu-devel] [PATCH RFC] error: Include hint everywhere

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Tue, 12/19 15:29, Markus Armbruster wrote:
>> Adding Eric for additonal QMP design expertise.
>> 
>> Fam Zheng <address@hidden> writes:
>> 
>> > Previously we only print hint lines if we are in a command line context
>> > or HMP. However QMP errors are also eventually consumed by human and the
>> > hint could help.
>> >
>> > Append hint lines already in error_get_pretty() and do as said above
>> > consistently in CLI, HMP and QMP.
>> >
>> > Signed-off-by: Fam Zheng <address@hidden>
>> 
>> Problematic.
>> 
>> The intended use of the "hint" feature is to add hints on the *human*
>> user interface.  These need not make sense for QMP, which has different
>> syntax.  Please see
>> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01991.html
>> (which I let fall through the cracks, sorry about that).
>> 
>> In practice, we've been using / abusing the feature for other purposes,
>> too, i.e. hints that do make sense for QMP.
>> 
>> If we decide we want to transmit such hints via QMP, we need to decide
>> how to (more on that below), and we need to examine the existing hints
>> one by one to decide whether they make sense for QMP.
>> 
>> If we determine that all of the existing hints make sense for QMP, and
>> expect that all future hints will, then we can repurpose
>> error_append_hint().  Else, we need two functions, one for each kind of
>> hint.
>> 
>> On to QMP design.  Quote qmp-spec.txt:
>> 
>>     2.4.2 error
>>     -----------
>> 
>>     The format of an error response is:
>> 
>>     { "error": { "class": json-string, "desc": json-string }, "id": 
>> json-value }
>> 
>>      Where,
>> 
>>     - The "class" member contains the error class name (eg. "GenericError")
>>     - The "desc" member is a human-readable error message. Clients should
>>       not attempt to parse this message.
>>     - The "id" member contains the transaction identification associated with
>>       the command execution if issued by the Client
>> 
>> Your patch changes "desc" from a short(ish) message without newline to a
>> multi-line message that may or may not end with a newline (I think).
>> Is that a good idea?
>
> Hmm, I didn't pay attention to the ending '\n', you are right we should keep 
> it
> consistent.
>
> Multiline is not a big problem IMO. Because, I'm inclined to think that for
> GenericError errors, desc are not for machines; the desc content is not part 
> of
> the API and we are free to update it.

"desc" being a short(ish) message without newline is de facto ABI.
A management application that logs messages could well be upset by a
switch to multiline.

A simple example is a log file consisting of lines with a fixed prefix
followed by free-form text.  Code writing such a log needs to be
prepared for newlines in free-form text, or else you'll get things like

    Dec 20 09:20:54 Some log entry
    Dec 20 09:24:14 Error message
    First line of hint
    Second line of hint
    Dec 20 10:01:05 Some other log entry

Two hint lines do not conform to the format.  Except when their text
matches the fixed format, but that's even worse, because then you can't
even identify the start of messages reliably.

> That said, the case where my patch would be inappropriate is when the error
> comes from a libvirt misconfiguration but the error message is QEMU-specific 
> and
> doesn't make sense in a libvirt (abstraction) context.
>
> So you are probably right we still need to distinguish them in the QMP level.
>
> Maybe the right thing to do is just convert any error_append_hint() to an
> updated error_setg() and already include the "hint" in the error message 
> proper.
> So that we can document that error_append_hint() is specifically used for the
> case said above, for example.

I'm not sure I understand what you're proposing to do.

>> The conservative way to add hints is a new optional member.
>
> I don't like that because it leaves the decision for how to handle the 
> optional
> information to the management layer, which, again, may not have the right
> information (whether displaying the message to users makes any sense).

Whether we encode error message and hint in a single string separated by
the first newline, or in two strings doesn't really change this problem,
does it?