[Qemu-devel] [PATCH 0/2] Silence test-vmstate errors

[Qemu-devel] [PATCH 0/2] Silence test-vmstate errors

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

In a1771070e7 I added an error message during migration loading
so you could see the field that failed to load.
Unfortunately this triggers during the expected-failure case
testing in tests/test-vmstate.

This silences that error by providing a dummy monitor pointer
that doesn't output anything and then selecting it around
the noisy part of the test.

(I couldn't find anywhere better than libqtest.h for the declaration
but stubs doesn't have it's own header; is it worth creating one
just for that?)

Dave

Dr. David Alan Gilbert (2):
  tests/stubs: Add a dummy, silent monitor
  test-vmstate: Silence expected errors

 stubs/mon-is-qmp.c   | 9 +++++++++
 tests/libqtest.h     | 1 +
 tests/test-vmstate.c | 8 ++++++++
 3 files changed, 18 insertions(+)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] tests/stubs: Add a dummy, silent monitor

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

We need an easy way to silence error_report's that come
up in meant-to-fail test cases.  The easiest way to do
that is to create a monitor instance, and since our stubbed
monitor_printf's are slent this causes the errors to disappear.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 stubs/mon-is-qmp.c | 9 +++++++++
 tests/libqtest.h   | 1 +
 2 files changed, 10 insertions(+)

diff --git a/stubs/mon-is-qmp.c b/stubs/mon-is-qmp.c
index a8344ce..012eb28 100644
--- a/stubs/mon-is-qmp.c
+++ b/stubs/mon-is-qmp.c
@@ -2,7 +2,16 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
+/* Monitor is defined internally to the real monitor.c, so
+ * it's real contents are never accessed when stubs are in use;
+ * just a pointer.
+ */
+struct Monitor {
+    int dummy;
+};
+
 Monitor *cur_mon;
+Monitor stubs_silent_monitor;
 
 bool monitor_cur_is_qmp(void)
 {
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 4be1f77..be34dbb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -897,4 +897,5 @@ void qmp_fd_send(int fd, const char *fmt, ...);
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
 QDict *qmp_fd(int fd, const char *fmt, ...);
 
+extern Monitor stubs_silent_monitor;
 #endif
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/2] tests/stubs: Add a dummy, silent monitor

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On 10/20/2016 05:36 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> We need an easy way to silence error_report's that come
> up in meant-to-fail test cases.  The easiest way to do
> that is to create a monitor instance, and since our stubbed
> monitor_printf's are slent this causes the errors to disappear.

s/slent/silent/

> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  stubs/mon-is-qmp.c | 9 +++++++++
>  tests/libqtest.h   | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/stubs/mon-is-qmp.c b/stubs/mon-is-qmp.c
> index a8344ce..012eb28 100644
> --- a/stubs/mon-is-qmp.c
> +++ b/stubs/mon-is-qmp.c
> @@ -2,7 +2,16 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>  
> +/* Monitor is defined internally to the real monitor.c, so

s/so/but/

> + * it's real contents are never accessed when stubs are in use;

s/it's/its/  ("it's" is only appropriate if "it is" also works)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/2] test-vmstate: Silence expected errors

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

test-vmstate has some expected failure tests (from explicitly
truncating an input stream); these trigger errors in the migration
code that are now reported;  silence these errors.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/test-vmstate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d8da26f..79cca87 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -27,8 +27,10 @@
 #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"
+#include "libqtest.h"
 
 static char temp_file[] = "/tmp/vmst.test.XXXXXX";
 static int temp_fd;
@@ -132,6 +134,9 @@ static int load_vmstate(const VMStateDescription *desc,
                         void (*obj_copy)(void *, void*),
                         int version, uint8_t *wire, size_t size)
 {
+    /* Silence errors during the expected failures */
+    cur_mon = &stubs_silent_monitor;
+
     /* We test with zero size */
     obj_copy(obj_clone, obj);
     FAILURE(load_vmstate_one(desc, obj, version, wire, 0));
@@ -154,6 +159,9 @@ static int load_vmstate(const VMStateDescription *desc,
         FAILURE(load_vmstate_one(desc, obj, version, wire + (size/2), size/2));
 
     }
+    /* Now we shouldn't get any more errors - go back to normal */
+    cur_mon = NULL;
+
     obj_copy(obj, obj_clone);
     return load_vmstate_one(desc, obj, version, wire, size);
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/2] test-vmstate: Silence expected errors

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> test-vmstate has some expected failure tests (from explicitly
> truncating an input stream); these trigger errors in the migration
> code that are now reported;  silence these errors.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/test-vmstate.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d8da26f..79cca87 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -27,8 +27,10 @@
>  #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"
> +#include "libqtest.h"
>  
>  static char temp_file[] = "/tmp/vmst.test.XXXXXX";
>  static int temp_fd;
> @@ -132,6 +134,9 @@ static int load_vmstate(const VMStateDescription *desc,
>                          void (*obj_copy)(void *, void*),
>                          int version, uint8_t *wire, size_t size)
>  {
> +    /* Silence errors during the expected failures */
> +    cur_mon = &stubs_silent_monitor;
> +
>      /* We test with zero size */
>      obj_copy(obj_clone, obj);
>      FAILURE(load_vmstate_one(desc, obj, version, wire, 0));
> @@ -154,6 +159,9 @@ static int load_vmstate(const VMStateDescription *desc,
>          FAILURE(load_vmstate_one(desc, obj, version, wire + (size/2), size/2));
>  
>      }
> +    /* Now we shouldn't get any more errors - go back to normal */

I feel this comment is of marginal value, especially once the pattern
becomes more widely used.

> +    cur_mon = NULL;
> +
>      obj_copy(obj, obj_clone);
>      return load_vmstate_one(desc, obj, version, wire, size);
>  }

Works.  My autopilot would do

       old_mon = cur_mon;
       cur_mon = &stubs_silent_monitor;

       ...

       cur_mon = old_mon

because it avoids making assumptions on cur_mon's value.  Pick what you
like better.

Re: [Qemu-devel] [PATCH 2/2] test-vmstate: Silence expected errors

[Paolo Bonzini]

On 24/10/2016 14:55, Markus Armbruster wrote:
> Works.  My autopilot would do
> 
>        old_mon = cur_mon;
>        cur_mon = &stubs_silent_monitor;
> 
>        ...
> 
>        cur_mon = old_mon
> 
> because it avoids making assumptions on cur_mon's value.  Pick what you
> like better.

I don't think we should rely on a monitor.  The assertion here is that
there was no error_report.

Paolo

Re: [Qemu-devel] [PATCH 0/2] Silence test-vmstate errors

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> In a1771070e7 I added an error message during migration loading
> so you could see the field that failed to load.
> Unfortunately this triggers during the expected-failure case
> testing in tests/test-vmstate.
>
> This silences that error by providing a dummy monitor pointer
> that doesn't output anything and then selecting it around
> the noisy part of the test.
>
> (I couldn't find anywhere better than libqtest.h for the declaration
> but stubs doesn't have it's own header; is it worth creating one
> just for that?)

Maybe.  Always hard to say for the first one.

You could simply put it in monitor.h.  When somebody uses it outside
tests, linking fails.  If that bothers you, feel free to implement a
silent non-stub monitor.

With Eric's spelling fixes squashed in, and with or without my
suggestion on PATCH 2:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] Silence test-vmstate errors

[Paolo Bonzini]

On 24/10/2016 14:59, Markus Armbruster wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> In a1771070e7 I added an error message during migration loading
>> so you could see the field that failed to load.
>> Unfortunately this triggers during the expected-failure case
>> testing in tests/test-vmstate.
>>
>> This silences that error by providing a dummy monitor pointer
>> that doesn't output anything and then selecting it around
>> the noisy part of the test.
>>
>> (I couldn't find anywhere better than libqtest.h for the declaration
>> but stubs doesn't have it's own header; is it worth creating one
>> just for that?)
> 
> Maybe.  Always hard to say for the first one.
> 
> You could simply put it in monitor.h.  When somebody uses it outside
> tests, linking fails.  If that bothers you, feel free to implement a
> silent non-stub monitor.
> 
> With Eric's spelling fixes squashed in, and with or without my
> suggestion on PATCH 2:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

We could also implement error_vprintf so that it calls g_test_message().
 That would improve the logging.

Paolo