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