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

[Halil Pasic]

On 06/07/2017 02:01 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) wrote:
>>
>>
>> On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (address@hidden) wrote:
>>>> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL
>>>> so the hint states the assertion probably failed due to a bug. Introduce
>>>> _EQUAL_HINT for specifying a context specific hint.
>>>>
>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>
>>> I'd prefer not to print 'Bug!?' by default - they already get the
>>> message telling them something didn't match and the migration fails.
>>> There are none-bug ways of this happening, e.g. a user starting a VM on
>>> the source and destination with different configs.
>>
>> I admit, my objective with 'Bug!?' was to provoke. My train of thought is
>> to encourage the programmer to think about and document the circumstances
>> under which such an assertion is supposed to fail (and against which it
>> is supposed to guard).
>>
>> I do not know how skillful are our users but a 4 != 5 then maybe a name
>> of a vmstate field is probably quite scary and not very revealing. I doubt
>> a non qemu developer can use it for something else that reporting a bug.
>>
>> Consequently if there are non-bug ways one can use the hint and state them.
>> Your example with the misconfigured target, by the way, is IMHO also be due
>> to a bug of the management software IMHO.
>>
>> To sum it up: IMHO the message provided by a failing _EQUAL is to ugly
>> and Qemuspeak to be presented to an user-user in non-bug cases. Agree?
>> Disagree?
> 
> Disagree.
> 
> I don't mind giving field names etc; they make it easy for us as
> developers to track down what's happening, but also sometimes they help
> endusers work around a prolem or see where the problem is; of course
> that varies depending on the field name, but some of our names are
> reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in
> vmmouse.c).  They're also pretty good if two end users hit the same
> problem they can see the same error message in a bug report.
> 
> We often have customer-facing support people look at logs before they
> get as far as us developers; if we have bugs that are 
> 'if it's a failing BLAH device complaining about the BAR field'
> then this fixes it, then that helps them find workarounds/fixes quickly
> even if they don't understand what the BAR field is.
> 

You seem to forget, that I'm not proposing omitting this information,
but extending it with something civilized so one can distinguish between
an assert failed should have never happened situation an a as good as
reasonable error handling for an expected error scenario. IMHO the current
EQUAL looks more like the former (assert) and less like the later (error
reporting for an expected error scenario). Agree? Dissagree?

Having a field name is great! That's beyond discussion.

I see, my 'sum it up' above was a bit unfortunate: it sounds like I'm
against the inclusion of technical info and not against a lack of non
technical info. Sorry for that!

>>
>>>
>>> (I also worry we have a lot f macros for each size;
>>> EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for
>>> that)
>>>
>>
>> If we are going to drop the default hint ('Bug?!' or whatever) then
>> I think we could just add an extra NULL hint to each existing  _EQUAL
>> usage, re-purpose EQUAL, and forget about introducing new _HINT macros.
>>
>> What to you think?
> 
> Yes, that would be a lot simpler; and there aren't that many
> VMSTATE*EQUAL* macros in use.
> 

I still have not given up on the discussion above. Will do depending
on the outcome.

Regards,
Halil