* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>
>
> On 06/22/2017 10:22 AM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) 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 (pa...@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 <pa...@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
> >>>
> >>>
> >>
> >> 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
> >
>
> Thanks for the reply! Since nobody else cared to explain the logic,
> I guess it is not all that important and we are fine with printing
> the hint in QMP context too.
>
> I would like to keep the output consistent with 8ed179c937 ("s390x/css:
> catch section mismatch on load", 2017-05-18).
>
> First I tried with (too make the err_hint look like a hint)
>
> + if (field->err_hint) {
> + error_report("%" PRIx32 " != %" PRIx32 "\n%s\n",
> + *v, v2, field->err_hint);
> + } else {
> + error_report("%" PRIx32 " != %" PRIx32, *v, v2);
> + }
>
> but checkpatch does not like that because newline in error
> message seems to be evil.
>
> Would you be also OK with:
>
> error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>
> if (field->err_hint) {
>
> error_printf("%s\n", field->err_hint);
>
> }
Yes I'm OK with that - what's important to me is getting the output
into the stderr log so I've got something to work with when it fails.
> Regards,
> or are you preferring producing a single line (in that case I
> would have to sacrifice 'no change in behavior' for my vmstate
> conversion of virtio-ccw :( )?
Single line is less important to me.
Dave
> Halil
>
>
> >> Regards,
> >> Halil
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
