* 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 > Regards, > Halil > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK