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

[Halil Pasic]

On 06/08/2017 01:05 PM, Halil Pasic wrote:
> 
> 
> On 06/07/2017 11:51 AM, Dr. David Alan Gilbert wrote:
>> * Halil Pasic (address@hidden) wrote:
>>> In some cases a failing VMSTATE_*_EQUAL does not mean we detected a bug
>>> (it's actually the best we can do). Especially in these cases a verbose
>>> error message is required.
>>>
>>> Let's introduce infrastructure for specifying a error hint to be used if
>>> equal check fails.
>>>
>>> Signed-off-by: Halil Pasic <address@hidden>
>>> ---
>>> Macros come in part 2. Once we are happy with the macros
>>> this two patches should be squashed into one. 
>>> ---
>>>  include/migration/vmstate.h |  1 +
>>>  migration/vmstate-types.c   | 36 +++++++++++++++++++++++++++++++-----
>>>  2 files changed, 32 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>> index 66895623da..d90d9b12ca 100644
>>> --- a/include/migration/vmstate.h
>>> +++ b/include/migration/vmstate.h
>>> @@ -200,6 +200,7 @@ typedef enum {
>>>  
>>>  struct VMStateField {
>>>      const char *name;
>>> +    const char *err_hint;
>>>      size_t offset;
>>>      size_t size;
>>>      size_t start;
>>> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>>> index 7287c6baa6..84d0545a38 100644
>>> --- a/migration/vmstate-types.c
>>> +++ b/migration/vmstate-types.c
>>> @@ -19,6 +19,7 @@
>>>  #include "qemu/error-report.h"
>>>  #include "qemu/queue.h"
>>>  #include "trace.h"
>>> +#include "qapi/error.h"
>>>  
>>>  /* bool */
>>>  
>>> @@ -118,6 +119,7 @@ const VMStateInfo vmstate_info_int32 = {
>>>  static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
>>>                             VMStateField *field)
>>>  {
>>> +    Error *err = NULL;
>>>      int32_t *v = pv;
>>>      int32_t v2;
>>>      qemu_get_sbe32s(f, &v2);
>>> @@ -125,7 +127,11 @@ static int get_int32_equal(QEMUFile *f, void *pv, 
>>> size_t size,
>>>      if (*v == v2) {
>>>          return 0;
>>>      }
>>> -    error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>>> +    error_setg(&err, "%" PRIx32 " != %" PRIx32, *v, v2);
>>> +    if (field->err_hint) {
>>> +        error_append_hint(&err, "%s\n", field->err_hint);
>>> +    }
>>> +    error_report_err(err);
>>
>> I'm a bit worried as to whether the error_append_hint data gets
>> printed out by error_report_err if we're being driven by a QMP
>> monitor.
>> error_report_err uses error_printf_unless_qmp
>>
>> Since this code doesn't really handle Error *'s back up,
>> and always prints it's errors into stderr, I'd prefer if you just
>> used error_report again for the hint, something like:
>>
>> if (field->err_hint) {
>>   error_report("%" PRIx32 " != %" PRIx32 "(%s)",
>>                *v, v2, field->err_hint);
>> } else {
>>   error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>> }
>>
>> Dave
> 
> 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?' and 'Why is this
case special (a hint should be reported
even in QMP context?'

Regarding the first question hints being reported via
error_printf_unless_qmp seems to come from commit
50b7b000c9 ("hmp: Allow for error message hints on HMP")
--> Cc-ing Eric maybe he can help.

Regards,
Halil