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);
}
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 :( )?
Regards,
Halil
>> Regards,
>> Halil
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
