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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Wed, 12/20 10:05, Markus Armbruster wrote:
>> 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.
>
> OK, if "no newline in desc" is the rule, then this doesn't work.

It's not a written rule.  I might be excessively cautious.

>> > 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.
>
> For example, change error_append_hint() callers in this pattern:
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 36ee89e940..a542479c95 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -703,11 +703,10 @@ static int raw_check_lock_bytes(BDRVRawState *s,
>              if (ret) {
>                  char *perm_name = bdrv_perm_names(p);
>                  error_setg(errp,
> -                           "Failed to get shared \"%s\" lock",
> +                           "Failed to get shared \"%s\" lock. "
> +                           "Is another process using the image?",
>                             perm_name);
>                  g_free(perm_name);
> -                error_append_hint(errp,
> -                                  "Is another process using the image?\n");
>                  return ret;
>              }
>          }

An error message should be a short phrase.  Multiple sentences are an
anti-pattern.

However, this particular one could perhaps be "Failed to get shared
\"%s\" lock (is another process using the image?)".  Might be pushing
"short" beyond its limit.

>> >> 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?
>
> On this, my idea is above. As the author of the --- code in the diff I'm fine
> with doing that given what the current error API does on QMP and that people
> complains when they don't see the hint from libvirt error message.
>
> So what I'm trying to say is that I think some messages added by
> error_append_hint() are worth exposing in QMP as well, and some are not. Do we
> want some wider changes somewhere rather than the above diff?

Quoting myself:

    If we decide we want to transmit [...] 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.

In other words:

0. Pick a name other than "hint" for this kind of hint, to avoid future
confusion.

1. Extend QMP error wire format to carry it

2. Create a C interface to add it

3. Convert existing error_append_hint() to it, where appropriate