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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 06/07/2017 11:51 AM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) 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 <pasic@linux.vnet.ibm.com>
>> ---
>> 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