[Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Peter Maydell]

I've just noticed that qemu master running 'make check' prints
  GTESTER tests/test-vmstate
Failed to load simple/primitive:b_1
Failed to load simple/primitive:i64_2
Failed to load simple/primitive:i32_1
Failed to load simple/primitive:i32_1

but the test doesn't fail.

Can we either (a) silence this output if it's spurious or (b) have
it cause the test to fail if it's real (and fix the cause of the
failure ;-)), please?

thanks
-- PMM

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> I've just noticed that qemu master running 'make check' prints
>   GTESTER tests/test-vmstate
> Failed to load simple/primitive:b_1
> Failed to load simple/primitive:i64_2
> Failed to load simple/primitive:i32_1
> Failed to load simple/primitive:i32_1
> 
> but the test doesn't fail.
> 
> Can we either (a) silence this output if it's spurious or (b) have
> it cause the test to fail if it's real (and fix the cause of the
> failure ;-)), please?

The test (has always) tried loading truncated versions of the migration
stream and made sure that it receives an error from vmstate_load_state.

However I just added an error so we can see which field fails to load
in a migration where we just used to get a 'migration has failed with -22'

Is there a way to silence error_report's that's already in use in tests?

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Peter Maydell]

On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> I've just noticed that qemu master running 'make check' prints
>>   GTESTER tests/test-vmstate
>> Failed to load simple/primitive:b_1
>> Failed to load simple/primitive:i64_2
>> Failed to load simple/primitive:i32_1
>> Failed to load simple/primitive:i32_1
>>
>> but the test doesn't fail.
>>
>> Can we either (a) silence this output if it's spurious or (b) have
>> it cause the test to fail if it's real (and fix the cause of the
>> failure ;-)), please?
>
> The test (has always) tried loading truncated versions of the migration
> stream and made sure that it receives an error from vmstate_load_state.
>
> However I just added an error so we can see which field fails to load
> in a migration where we just used to get a 'migration has failed with -22'
>
> Is there a way to silence error_report's that's already in use in tests?

We have some nasty hacks (like check for 'qtest_enabled()' before
calling error_report()) but we don't have anything in the
tree today that's a more coherent approach to the "test
deliberately provoked this error" problem.

thanks
-- PMM

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> I've just noticed that qemu master running 'make check' prints
> >>   GTESTER tests/test-vmstate
> >> Failed to load simple/primitive:b_1
> >> Failed to load simple/primitive:i64_2
> >> Failed to load simple/primitive:i32_1
> >> Failed to load simple/primitive:i32_1
> >>
> >> but the test doesn't fail.
> >>
> >> Can we either (a) silence this output if it's spurious or (b) have
> >> it cause the test to fail if it's real (and fix the cause of the
> >> failure ;-)), please?
> >
> > The test (has always) tried loading truncated versions of the migration
> > stream and made sure that it receives an error from vmstate_load_state.
> >
> > However I just added an error so we can see which field fails to load
> > in a migration where we just used to get a 'migration has failed with -22'
> >
> > Is there a way to silence error_report's that's already in use in tests?
> 
> We have some nasty hacks (like check for 'qtest_enabled()' before
> calling error_report()) but we don't have anything in the
> tree today that's a more coherent approach to the "test
> deliberately provoked this error" problem.

Errors go to either the current monitor (if it's non-qmp) or
stderr; so could we create a dummy monitor to eat the errors
and make it current around that part?

Dave

> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Paolo Bonzini]

On 17/10/2016 21:15, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>>> I've just noticed that qemu master running 'make check' prints
>>>>   GTESTER tests/test-vmstate
>>>> Failed to load simple/primitive:b_1
>>>> Failed to load simple/primitive:i64_2
>>>> Failed to load simple/primitive:i32_1
>>>> Failed to load simple/primitive:i32_1
>>>>
>>>> but the test doesn't fail.
>>>>
>>>> Can we either (a) silence this output if it's spurious or (b) have
>>>> it cause the test to fail if it's real (and fix the cause of the
>>>> failure ;-)), please?
>>>
>>> The test (has always) tried loading truncated versions of the migration
>>> stream and made sure that it receives an error from vmstate_load_state.
>>>
>>> However I just added an error so we can see which field fails to load
>>> in a migration where we just used to get a 'migration has failed with -22'
>>>
>>> Is there a way to silence error_report's that's already in use in tests?
>>
>> We have some nasty hacks (like check for 'qtest_enabled()' before
>> calling error_report()) but we don't have anything in the
>> tree today that's a more coherent approach to the "test
>> deliberately provoked this error" problem.
> 
> Errors go to either the current monitor (if it's non-qmp) or
> stderr; so could we create a dummy monitor to eat the errors
> and make it current around that part?

I guess you could reimplement the functions of stubs/mon-printf.c and
stubs/mon-is-qmp.c.

Paolo

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 17/10/2016 21:15, Dr. David Alan Gilbert wrote:
>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>>>> I've just noticed that qemu master running 'make check' prints
>>>>>   GTESTER tests/test-vmstate
>>>>> Failed to load simple/primitive:b_1
>>>>> Failed to load simple/primitive:i64_2
>>>>> Failed to load simple/primitive:i32_1
>>>>> Failed to load simple/primitive:i32_1
>>>>>
>>>>> but the test doesn't fail.
>>>>>
>>>>> Can we either (a) silence this output if it's spurious or (b) have
>>>>> it cause the test to fail if it's real (and fix the cause of the
>>>>> failure ;-)), please?
>>>>
>>>> The test (has always) tried loading truncated versions of the migration
>>>> stream and made sure that it receives an error from vmstate_load_state.
>>>>
>>>> However I just added an error so we can see which field fails to load
>>>> in a migration where we just used to get a 'migration has failed with -22'
>>>>
>>>> Is there a way to silence error_report's that's already in use in tests?
>>>
>>> We have some nasty hacks (like check for 'qtest_enabled()' before
>>> calling error_report()) but we don't have anything in the
>>> tree today that's a more coherent approach to the "test
>>> deliberately provoked this error" problem.

I guess the "more coherent approach" would be some way to run a piece of
code with error reporting suppressed.

For unit tests, a need to supress error reporting indicates the code
under test should perhaps error_setg() instead of error_report().
Unlikely to completely eliminate the need to suppress error reporting,
though.

>> Errors go to either the current monitor (if it's non-qmp) or
>> stderr; so could we create a dummy monitor to eat the errors
>> and make it current around that part?
>
> I guess you could reimplement the functions of stubs/mon-printf.c and
> stubs/mon-is-qmp.c.

qemu-error.c does all its output via error_vprintf():

    void error_vprintf(const char *fmt, va_list ap)
    {
        if (cur_mon && !monitor_cur_is_qmp()) {
            monitor_vprintf(cur_mon, fmt, ap);
        } else {
            vfprintf(stderr, fmt, ap);
        }
    }

Thus, custom monitor_cur_is_qmp() and monitor_vprintf() let you capture
its output.

Using (or rather abusing) this to suppress error reporting for an entire
program would be easy enough.  But I doubt it's a convenient way to
suppress more narrowly.

I figure a monitor connected to a "null" chardev would work better
there.  If we need that anyway, using it for the "entire program" case
as well is probably easiest.

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 17/10/2016 21:15, Dr. David Alan Gilbert wrote:
> >> * Peter Maydell (peter.maydell@linaro.org) wrote:
> >>> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> >>>> * Peter Maydell (peter.maydell@linaro.org) wrote:
> >>>>> I've just noticed that qemu master running 'make check' prints
> >>>>>   GTESTER tests/test-vmstate
> >>>>> Failed to load simple/primitive:b_1
> >>>>> Failed to load simple/primitive:i64_2
> >>>>> Failed to load simple/primitive:i32_1
> >>>>> Failed to load simple/primitive:i32_1
> >>>>>
> >>>>> but the test doesn't fail.
> >>>>>
> >>>>> Can we either (a) silence this output if it's spurious or (b) have
> >>>>> it cause the test to fail if it's real (and fix the cause of the
> >>>>> failure ;-)), please?
> >>>>
> >>>> The test (has always) tried loading truncated versions of the migration
> >>>> stream and made sure that it receives an error from vmstate_load_state.
> >>>>
> >>>> However I just added an error so we can see which field fails to load
> >>>> in a migration where we just used to get a 'migration has failed with -22'
> >>>>
> >>>> Is there a way to silence error_report's that's already in use in tests?
> >>>
> >>> We have some nasty hacks (like check for 'qtest_enabled()' before
> >>> calling error_report()) but we don't have anything in the
> >>> tree today that's a more coherent approach to the "test
> >>> deliberately provoked this error" problem.
> 
> I guess the "more coherent approach" would be some way to run a piece of
> code with error reporting suppressed.
> 
> For unit tests, a need to supress error reporting indicates the code
> under test should perhaps error_setg() instead of error_report().
> Unlikely to completely eliminate the need to suppress error reporting,
> though.
> 
> >> Errors go to either the current monitor (if it's non-qmp) or
> >> stderr; so could we create a dummy monitor to eat the errors
> >> and make it current around that part?
> >
> > I guess you could reimplement the functions of stubs/mon-printf.c and
> > stubs/mon-is-qmp.c.
> 
> qemu-error.c does all its output via error_vprintf():
> 
>     void error_vprintf(const char *fmt, va_list ap)
>     {
>         if (cur_mon && !monitor_cur_is_qmp()) {
>             monitor_vprintf(cur_mon, fmt, ap);
>         } else {
>             vfprintf(stderr, fmt, ap);
>         }
>     }
> 
> Thus, custom monitor_cur_is_qmp() and monitor_vprintf() let you capture
> its output.
> 
> Using (or rather abusing) this to suppress error reporting for an entire
> program would be easy enough.  But I doubt it's a convenient way to
> suppress more narrowly.
> 
> I figure a monitor connected to a "null" chardev would work better
> there.  If we need that anyway, using it for the "entire program" case
> as well is probably easiest.

I've not quite figured it out but we're linked against the stubs/ monitor
code rather than the real monitor code; and the stubs monitor_vprintf discards
and the stubs monitor_cur_is_qmp() always returns false, so to silence things
we just have to make cur_mon non-NULL.
Unfortunately cur_mon is of type Monitor * and Monitor isn't defined publicly,
so we can't know it's size.  The following evil hack does silence things
for anyone desperate, but I do need to find a neater way; perhaps the right
thing is just to link against monitor and create a dummy "null" chardev as you
say.

Dave

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d8da26f..c39d3dc 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -27,6 +27,7 @@
 #include "qemu-common.h"
 #include "migration/migration.h"
 #include "migration/vmstate.h"
+#include "monitor/monitor.h"
 #include "qemu/coroutine.h"
 #include "io/channel-file.h"
 
@@ -134,6 +135,11 @@ static int load_vmstate(const VMStateDescription *desc,
 {
     /* We test with zero size */
     obj_copy(obj_clone, obj);
+    /* NASTY HACK! Silence error_report - relies on the fact we're using the
+     * stubs code rather than the real monitor.  As long as cur_mon is
+     * non-null error_report won't print.
+     */
+    cur_mon = g_malloc(1);
     FAILURE(load_vmstate_one(desc, obj, version, wire, 0));
 
     /* Stream ends with QEMU_EOF, so we need at least 3 bytes to be
@@ -154,6 +160,9 @@ static int load_vmstate(const VMStateDescription *desc,
         FAILURE(load_vmstate_one(desc, obj, version, wire + (size/2), size/2));
 
     }
+    /* Unsilence error-report - we shouldn't get any errors here */
+    g_free(cur_mon);
+    cur_mon = NULL;
     obj_copy(obj, obj_clone);
     return load_vmstate_one(desc, obj, version, wire, size);
 }

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Paolo Bonzini]

On 18/10/2016 11:54, Dr. David Alan Gilbert wrote:
> I've not quite figured it out but we're linked against the stubs/ monitor
> code rather than the real monitor code; and the stubs monitor_vprintf discards
> and the stubs monitor_cur_is_qmp() always returns false, so to silence things
> we just have to make cur_mon non-NULL.
> Unfortunately cur_mon is of type Monitor * and Monitor isn't defined publicly,
> so we can't know it's size.  The following evil hack does silence things
> for anyone desperate, but I do need to find a neater way; perhaps the right
> thing is just to link against monitor and create a dummy "null" chardev as you
> say.

If error_printf/error_vprintf are to a separate file, then stubs/ can be
changed to use vfprintf unconditionally.

And then I wonder what we actually use cur_mon for, perhaps with this
change we can remove stubs/mon*.

Paolo

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 18/10/2016 11:54, Dr. David Alan Gilbert wrote:
> > I've not quite figured it out but we're linked against the stubs/ monitor
> > code rather than the real monitor code; and the stubs monitor_vprintf discards
> > and the stubs monitor_cur_is_qmp() always returns false, so to silence things
> > we just have to make cur_mon non-NULL.
> > Unfortunately cur_mon is of type Monitor * and Monitor isn't defined publicly,
> > so we can't know it's size.  The following evil hack does silence things
> > for anyone desperate, but I do need to find a neater way; perhaps the right
> > thing is just to link against monitor and create a dummy "null" chardev as you
> > say.
> 
> If error_printf/error_vprintf are to a separate file, then stubs/ can be
> changed to use vfprintf unconditionally.

Moving code out of util/qemu-error.c just so they can be stubbed separately
seems a little odd.

> And then I wonder what we actually use cur_mon for, perhaps with this
> change we can remove stubs/mon*.

I've just posted a slightly cleaner version of that nasty hack that gives
a value to assign to cur_mon.

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Paolo Bonzini]

On 20/10/2016 12:41, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>
>>
>> On 18/10/2016 11:54, Dr. David Alan Gilbert wrote:
>>> I've not quite figured it out but we're linked against the stubs/ monitor
>>> code rather than the real monitor code; and the stubs monitor_vprintf discards
>>> and the stubs monitor_cur_is_qmp() always returns false, so to silence things
>>> we just have to make cur_mon non-NULL.
>>> Unfortunately cur_mon is of type Monitor * and Monitor isn't defined publicly,
>>> so we can't know it's size.  The following evil hack does silence things
>>> for anyone desperate, but I do need to find a neater way; perhaps the right
>>> thing is just to link against monitor and create a dummy "null" chardev as you
>>> say.
>>
>> If error_printf/error_vprintf are to a separate file, then stubs/ can be
>> changed to use vfprintf unconditionally.
> 
> Moving code out of util/qemu-error.c just so they can be stubbed separately
> seems a little odd.

Why?  It is part of how static libraries work, and allowing fine-grained
inclusion is the reason why libqemustub.a and libqemuutil.a are static
libraries.

Paolo

>> And then I wonder what we actually use cur_mon for, perhaps with this
>> change we can remove stubs/mon*.
> 
> I've just posted a slightly cleaner version of that nasty hack that gives
> a value to assign to cur_mon.
> 
> Dave
> 
>> Paolo
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

[Halil Pasic]

On 10/18/2016 10:03 AM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> > On 17/10/2016 21:15, Dr. David Alan Gilbert wrote:
>>> >> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>>> >>> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>>>> >>>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>>>>> >>>>> I've just noticed that qemu master running 'make check' prints
>>>>>> >>>>>   GTESTER tests/test-vmstate
>>>>>> >>>>> Failed to load simple/primitive:b_1
>>>>>> >>>>> Failed to load simple/primitive:i64_2
>>>>>> >>>>> Failed to load simple/primitive:i32_1
>>>>>> >>>>> Failed to load simple/primitive:i32_1
>>>>>> >>>>>
>>>>>> >>>>> but the test doesn't fail.
>>>>>> >>>>>
>>>>>> >>>>> Can we either (a) silence this output if it's spurious or (b) have
>>>>>> >>>>> it cause the test to fail if it's real (and fix the cause of the
>>>>>> >>>>> failure ;-)), please?
>>>>> >>>>
>>>>> >>>> The test (has always) tried loading truncated versions of the migration
>>>>> >>>> stream and made sure that it receives an error from vmstate_load_state.
>>>>> >>>>
>>>>> >>>> However I just added an error so we can see which field fails to load
>>>>> >>>> in a migration where we just used to get a 'migration has failed with -22'
>>>>> >>>>
>>>>> >>>> Is there a way to silence error_report's that's already in use in tests?
>>>> >>>
>>>> >>> We have some nasty hacks (like check for 'qtest_enabled()' before
>>>> >>> calling error_report()) but we don't have anything in the
>>>> >>> tree today that's a more coherent approach to the "test
>>>> >>> deliberately provoked this error" problem.
> I guess the "more coherent approach" would be some way to run a piece of
> code with error reporting suppressed.
> 
> For unit tests, a need to supress error reporting indicates the code
> under test should perhaps error_setg() instead of error_report().
> Unlikely to completely eliminate the need to suppress error reporting,
> though.
> 
+1

For this particular case, I think it is viable too.

Halil