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

[Halil Pasic]

On 06/29/2017 09:04 PM, Eric Blake wrote:
> On 06/14/2017 08:51 AM, Halil Pasic wrote:
> 
> [apologies for the delayed response, and also adding Markus]
> 

No problem. Many thanks for the effort. I see I've ended up with a
lengthy email. A disclaimer before I start: No strong opinions here.
Things have been working reasonably well for years and I respect that.
Nevertheless I like conceptual clarity, and because of this, I ended up
doing discussion without considering the expected cost/benefit ration. If
I think about it that way it probably ain't wort it. So I'm OK with
concluding the discussion with that argument at any time -- just tell ;).


>>>
>>> One reason I choose error_report_err is to be consistent about hint
>>> reporting (the other one is that was what Connie suggested). I do
>>> not understand why do we omit hints if QMP, but I figured that's
>>> our policy. So the hint I'm adding must not be printed in QMP
>>> context -- because that's our policy. I was pretty sure what I
>>> want to do is add a hint (and not make a very long 'core' error
>>> message).
>>>
>>> Can you (or somebody else)  explain why are hints dropped in QMP
>>> context?
>>>
>>> Don't misunderstand I'm open towards your proposal, it's just
>>> that:
>>> 1) I would like to understand.
>>> 2) I would like to get the very same result as produced by
>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg01472.html 
>>>
>>> Regards,
>>> Halil
>>>
>>>
>>
>> ping.
>>
>> I would like to do a v2, but I want this sorted out first.
>>
>> '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?

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?

Let us also examine some comments in qapi/error.h:

/*
 * Just like error_setg(), except you get to specify the error class.
 * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
 * strongly discouraged.
 */
#define error_set(errp, err_class, fmt, ...)

This probably means client code (e.g. libvirt) is in general not meant
to make decision based on the type of the error that occurred (e.g. what
went wrong).

/*
[..]
 * human-readable error message is made from printf-style @fmt, ...
 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.
[..]
 */
#define error_setg(errp, fmt, ...) 

From this it seems to me error message is intended for human-consumption.

/*
 * Append a printf-style human-readable explanation to an existing error.
 * @errp may be NULL, but not &error_fatal or &error_abort.
 * Trivially the case if you call it only after error_setg() or
 * error_propagate().
 * May be called multiple times.  The resulting hint should end with a
 * newline.
 */
void error_append_hint(Error **errp, const char *fmt, ...)

From this, I would say: The 'hint' is about why something went wrong. The
'message' is about what problem in particular or in general was
encountered (in general, the requested operation failed/can not be
performed; the caller knows what operation was attempted) and should be
considered debugging aid (along with the bits supposed to answer the
question 'where'). This debugging aid, however, can be very useful to the
end user if seeking a workaround, and the error_class is for providing
client code with additional information beyond 'something went wrong'.

Whether the message is supposed to be only about 'in particular' is a
tricky one, and should probably depend on the contract: if the client
code is supposed tell us which high level operation failed then I guess
just 'in particular' is good, if however the client code is expected to
just log errors and proceed without providing any extra context then I
guess the message is both about 'in general' in particular. I think
error_prepend is used to provide the 'extra context' and shows in the
direction of the later, but I'm not sure (e.g. whether is it OK ton not
include any information about what where we trying to accomplish in the
message when an error is created).


> 
> There's also the question of whether the hints are even useful (telling
> the user to do something differently doesn't help if it wasn't the user,
> but libvirt, that was doing things wrong to cause the error in the first
> place).
> 

To me this translates to the following question. Is it reasonable to
assume that we are interested in what went wrong (error message).


> So while those points may or may not be the original rationale for why
> hints are not used in QMP, but it is an explanation that works for me
> now.  Markus may also have an opinion on the matter.
> 
>> and 'Why is this
>> case special (a hint should be reported
>> even in QMP context?'
> 
> If something absolutely must be reported, then it is not a hint, and
> shouldn't be using the hint mechanism.
> 

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

Regards,
Halil

signature.asc
Description: OpenPGP digital signature