qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dr. David Alan Gilbert
Subject: Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks
Date: Thu, 22 Jun 2017 09:22:07 +0100
User-agent: Mutt/1.8.2 (2017-04-18)

* Halil Pasic (address@hidden) wrote:
> 
> 
> 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.

I don't understand the full logic behind error_append_hint;
my only concern here is that the full text ends up on stderr
even if the migration is driven by QMP.
Since we can do that just by using error_report like it's already
being used with the slight change I suggested, it seems easy.

Dave

> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK



reply via email to

[Prev in Thread] Current Thread [Next in Thread]