[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
- Re: [Qemu-devel] [RFC PATCH 2/3] vmstate: error hint for failed equal checks part 2, (continued)
Re: [Qemu-devel] [RFC PATCH 2/3] vmstate: error hint for failed equal checks part 2, Juan Quintela, 2017/06/07
[Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks, Halil Pasic, 2017/06/06
Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks, Eric Blake, 2017/06/29
Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks, Halil Pasic, 2017/06/30
Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks, Eric Blake, 2017/06/30
Re: [Qemu-devel] [RFC PATCH 1/3] vmstate: error hint for failed equal checks, Halil Pasic, 2017/06/30
Re: [Qemu-devel] [RFC PATCH 3/3] s390x/css: add hint for devno missmatch, Halil Pasic, 2017/06/06