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

[Fam Zheng <famz@redhat.com>]

On Tue, 12/19 15:29, Markus Armbruster wrote:
> Adding Eric for additonal QMP design expertise.
> 
> Fam Zheng <famz@redhat.com> 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 <famz@redhat.com>
> 
> 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.

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.

> 
> 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).

Fam