Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks

[Markus Armbruster]

Halil Pasic <address@hidden> writes:

> On 06/30/2017 04:54 PM, Eric Blake wrote:
>> On 06/30/2017 09:41 AM, Halil Pasic wrote:
>>>>> 'This' basically boils down to the question and
>>>>> 'Why aren't hints reported in QMP context?'
>>>>
>>>> QMP is supposed to be machine-parseable.  Hints are supposed to be
>>>> human-readable. If you have a machine managing the monitor, the hint
>>>> adds nothing but bandwidth consumption, because machine should not be
>>>> parsing the human portion of the error message in the first place (as it
>>>> is, libvirt already just logs the human-readable portion of a message,
>>>> and bases its actions solely on the machine-stable portions of an error
>>>> reply: namely, whether an error was sent at all, and occasionally, what
>>>> error class was used for that error - there's no guarantee a human will
>>>> be reading the log, though).
>>>
>>>
>>> Seems I've made wrong assumptions about error messages (in QEMU) up until
>>> now. If I understand you correctly, in QEMU error messages are part of
>>> the API (but hints are not). Thus if one changes a typo in an error
>>> message (like here
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg06732.html) the
>>> one is strictly speaking breaking API backward compatibility.  Is that
>>> really the way we want to have things?
>> 
>> Quite the opposite. In QMP, the EXISTENCE of an error message is part of
>> the API, but the CONTENTS of the message are not (machines are not
>> supposed to further parse the message) - anything that the machine would
>> want to differentiate between two different possible error messages
>> should instead be conveyed via a second field in the same returned
>> dictionary (the error class), and not by parsing the message.  
>
> I think we are in agreement, it's just that you call 'error message' what
> I would call 'error response' (from docs/qmp-spec.txt).

According to qmp-spec.txt, the 'error response' is a JSON object of the
form

    { "error": { "class": json-string, "desc": json-string },
      "id": json-value }

>                                                         For me an error
> response MAY OR MAY NOT or MUST (I don't know it is not stated in
> qmp-spec.txt, and qapi-schema.json did not make me much smarter: I would
> guess may or may not -- there is even some comment in qapi-schema showing
> it that direction) contain a 'desc' which is per definition "- The
> "desc" member is a human-readable error message. Clients should not
> attempt to parse this message.".

Both in qmp-spec.txt and in the QAPI schema, members are mandatory
unless marked optional.  Thus, "desc" is mandatory.

> So I would call that 'error message'. If the logic (modulo reporting) in
> libvirt (I don't know, my focus isn't libvirt) or any other management
> software depends on the EXISTENCE of 'desc' (or human-readable portion of
> some error API object) I find that weird, but it's a definition thing.

QMP clients such as libvirt may depend on the existence of "desc", just
not on its contents.

Depending on existence: show it to a human user, log it ...

Depending on contents: if "desc" matches /pattern/, do this, else do
that.

>> Most
>> often, there is not a strong case for having differentiation, so most
>> errors are lumped in the generic class (error_setg() makes this easy to
>> do by default).  An example where differentiation matters: look at the
>> "Important Note" in blockdev.c:qmp_block_commit().
>
> I think I have seen that. I find the 'strong discouragement' weird, because
> if there is a reason to have differentiation the error class is the way
> to go. And if there is no reason to -- it should be obvious.

The "strong discouragement" is the result of a long and somewhat
tortuous history.  If you're interested, I can tell it once again.

>>> From prior experiences I'm more used to think about error messages as
>>> something meant for human consumption, and expressing things expected to
>>> be relevant for some kind of client code in a different way (optimized
>>> for machine consumption).
>>>
>>> If however the error message ain't part of the machine relevant portion,
>>> then the same argument applies as to the 'hint', and I don't see the
>>> reason for handling hints differently. Do you agree with my
>>> argumentation?
>> 
>> Indeed, it may not hurt to start passing the hints over the wire (errors
>> would then consume more bandwidth, but errors are not the hot path).
>> And I'm not necessarily opposed to that change, so much as trying to
>> document why it is not currently the case.  At the same time, I probably
>> won't be the one writing a path to populate the hint information into
>> the QMP error, as I don't have any reason to use the hint when
>> controlling libvirt (except maybe for logging, but there, the hint is
>> not going to help the end user, because it's not the end-user's fault
>> that libvirt used the API wrong to get a hint in the first place).
>
> For me both human readable things make sense only for error reporting
> (effectively logging). Error.msg should IMHO be different, than Error.hint.
> The existence of an error should be indicated by the Error object.

Consider this one from qemu-option.c:

        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
                   "a non-negative number below 2^64");
        error_append_hint(errp, "Optional suffix k, M, G, T, P or E means"
                          " kilo-, mega-, giga-, tera-, peta-\n"
                          "and exabytes, respectively.\n");

The hint is helpful for a human command line or HMP user.  It's actively
misleading in QMP.  Totally fine, it's how the "hint" feature is meant
to be used.

If we have errors that can't be adequately explained in a single error
message, we may need a way to add more explanation.  error_append_hint()
isn't.

>>>> If something absolutely must be reported, then it is not a hint, and
>>>> shouldn't be using the hint mechanism.

Exactly.

>>> I find it hard to formulate criteria for 'must be reported'. I'm afraid
>>> this is backwards logic: since the hint may not be reported everything
>>> that needs to be reported is not a hint. This is a valid approach of
>>> course, but then I think some modifications to the comments in error.h
>>> would not hurt. And maybe something with verbose would be more
>>> expressive name.
>>>
>>> I hope all this makes some sense and ain't pure waste of time...
>> 
>> No, it never hurts to question whether the design is optimal, and it's
>> better to question first to know whether it is even worth patching
>> things to behave differently, rather than spending time patching it only
>> to have a maintainer clarify that the patch can't be accepted because of
>> some design constraint.  So I still hope Markus will chime in.
>> 
>
> For this patch I went with Dave's proposal so I have no acute interest
> in changing this.
>
> Conceptually, for me it really boils down to the question: Is it reasonable
> to assume that we are interested in what went wrong (error message)?
>
> If yes, we are good as is. If no, we should not drop hint in QMP context.
>
> Thanks for your time. I think we provided Markus with enough input to
> make his call :).

I had a quick peek at the patch that triggered this discussion.  What
problem are you trying to solve?  According to your cover letter, it's
"to specify a hint for the case a vmstate equal assertion".  How is
nicer assertion failures related to QMP?  Am I confused?