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

[Halil Pasic]

On 07/04/2017 08:42 AM, Markus Armbruster wrote:
> Halil Pasic <address@hidden> writes:
> 
>> On 07/03/2017 03:52 PM, Markus Armbruster wrote:
>>> 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:
[..]
>>> 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.
>>>
>>
>> Was not aware. Using hint in this very situation was suggested by Connie,
>> and I assumed she is long enough with the project to know...
>>
>> In fact looking at  include/qapi/error.h:
>> """
>> /*
>>  * Error reporting system loosely patterned after Glib's GError.
>>  *
>>  * Create an error:
>>  *     error_setg(&err, "situation normal, all fouled up");
>>  *
>>  * Create an error and add additional explanation:
>>  *     error_setg(&err, "invalid quark");
>>  *     error_append_hint(&err, "Valid quarks are up, down, strange, "
>>  *                       "charm, top, bottom.\n");
>>  *
>>  * Do *not* contract this to
>>  *     error_setg(&err, "invalid quark\n"
>>  *                "Valid quarks are up, down, strange, charm, top, bottom.");
>> """
>>
>> my understanding was and is still the exact opposite of what you say:
>> error_append_hint is for adding more explanation.
>>
>> Furthermore 
>> """
>> /*
>>  * 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, ...)
>> """
>>
>> Assuming that error_append_hint() isn't for adding more explanation,
>> IMHO the doc does not adequately explain what it is for.
> 
> You're right, it doesn't.
> 
>> I have also failed to find any hint in qapi/error.h which is AFAIU
>> documenting the error api about this human-readable explanation
>> appended to an existing error by error_append_hint() is to be discarded
>> if the error is reported in QMP context.
>>
>> Am I reading the api doc incorrectly, or did the documentation and
>> de-facto api diverge (behavior)?
> 
> I added documentation after I inherited this subsystem, in response to
> recurring questions on proper use of the interface.  I failed to fully
> capture the hint feature's intent.  I'll post a patch.
> 

Hey, IMHO error.h is one of the better documented corners of the QEMU
code base. If you like put me on cc for this promised patch.

>>>>>>> If something absolutely must be reported, then it is not a hint, and
>>>>>>> shouldn't be using the hint mechanism.
>>>
>>> Exactly.
>>>
>>
>> Perfectly fine with me provided the apidoc tells me clearly what the hint is
>> for, and what it is not for.
>>
>>>>>> 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?
>>
>>
>> The problem is solved by d2164ad ("vmstate: error hint for failed equal
>> checks", 2017-06-23).
> 
> The way the commit uses error_report() and error_printf() looks good to
> me.
> 

I'm glad to hear that. My confidence ain't very high because my understand
of the infrastructure is shallow (especially the interface between
libvirt/management software and qemu and end user). Because of that
I may have relied on the api-doc more that the more knowledgeable colleagues.

>> The assertions ain't assertions in sense of the C programming
>> language. Maybe calling these 'checks' instead of 'assertions' in the
>> cover letter (like in the subject) would have been better. If one of
>> these 'assertions' fail qemu is supposed to abort the initiated load
>> (migration), state the reason, and terminate normally. In this sense these
>> 'assertions' are similar to the assertions in our unit tests (those fail
>> a test, and similarly to these do not terminate the program).
>>
>> The problem I was trying to solve is that the message generated by these
>> checks looked something like "5 != 4" which is OK if the check is never
>> supposed to fail, but not satisfactory for something we have to live
>> with.
>>
>> Sorry for the confusion.
> 
> No problem :)
> 

I'm glad all resolves positively.

Thanks,
Halil