qemu master tests/vmstate prints "Failed to load simple/primitive:b_1" etc

Commit Message

Dr. David Alan Gilbert Oct. 18, 2016, 9:54 a.m. UTC

* Markus Armbruster ([e-mail]) wrote:
> Paolo Bonzini <[e-mail]> writes:
> 
> > On 17/10/2016 21:15, Dr. David Alan Gilbert wrote:
> >> * Peter Maydell ([e-mail]) wrote:
> >>> On 17 October 2016 at 19:51, Dr. David Alan Gilbert <[e-mail]> wrote:
> >>>> * Peter Maydell ([e-mail]) 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


--
Dr. David Alan Gilbert / [e-mail] / Manchester, UK

Comments

Paolo Bonzini Oct. 18, 2016, 1:38 p.m. UTC | #1

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

Dr. David Alan Gilbert Oct. 20, 2016, 10:41 a.m. UTC | #2

* Paolo Bonzini ([e-mail]) 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 / [e-mail] / Manchester, UK

Paolo Bonzini Oct. 24, 2016, 2:26 p.m. UTC | #3

On 20/10/2016 12:41, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini ([e-mail]) 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 / [e-mail] / Manchester, UK
>

Patch

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);
 }