[Qemu-devel] [PATCH 0/2] tests: silence error_report in tests

[Qemu-devel] [PATCH 0/2] tests: silence error_report in tests

[Paolo Bonzini]

This is my (counter)proposal for silencing test-vmstate.

Thanks,

Paolo

Paolo Bonzini (2):
  qemu-error: remove dependency of stubs on monitor
  tests: send error_report to test log

 include/glib-compat.h       | 13 +++++++++++++
 include/qemu/error-report.h |  1 +
 monitor.c                   | 21 +++++++++++++++++++++
 stubs/Makefile.objs         |  2 +-
 stubs/error-printf.c        | 19 +++++++++++++++++++
 stubs/mon-printf.c          | 11 -----------
 util/qemu-error.c           | 26 +++-----------------------
 7 files changed, 58 insertions(+), 35 deletions(-)
 create mode 100644 stubs/error-printf.c
 delete mode 100644 stubs/mon-printf.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] qemu-error: remove dependency of stubs on monitor

[Paolo Bonzini]

Leave the implementation of error_vprintf and error_vprintf_unless_qmp
(the latter now trivially wrapped by error_printf_unless_qmp) to
libqemustub.a and monitor.c.  This has two advantages: it lets us
remove the monitor_printf and monitor_vprintf stubs, and it lets
tests provide a different implementation of the functions that uses
g_test_message.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/error-report.h |  1 +
 monitor.c                   | 21 +++++++++++++++++++++
 stubs/Makefile.objs         |  2 +-
 stubs/error-printf.c        | 13 +++++++++++++
 stubs/mon-printf.c          | 11 -----------
 util/qemu-error.c           | 26 +++-----------------------
 6 files changed, 39 insertions(+), 35 deletions(-)
 create mode 100644 stubs/error-printf.c
 delete mode 100644 stubs/mon-printf.c

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 499ec8b..3001865 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -32,6 +32,7 @@ void loc_set_file(const char *fname, int lno);
 
 void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_set_progname(const char *argv0);
 void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
diff --git a/monitor.c b/monitor.c
index b73a999..a02c883 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3957,6 +3957,27 @@ static void monitor_readline_flush(void *opaque)
     monitor_flush(opaque);
 }
 
+/*
+ * Print to current monitor if we have one, else to stderr.
+ * TODO should return int, so callers can calculate width, but that
+ * requires surgery to monitor_vprintf().  Left for another day.
+ */
+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);
+    }
+}
+
+void error_vprintf_unless_qmp(const char *fmt, va_list ap)
+{
+    if (cur_mon && !monitor_cur_is_qmp()) {
+        monitor_vprintf(cur_mon, fmt, ap);
+    }
+}
+
 static void __attribute__((constructor)) monitor_lock_init(void)
 {
     qemu_mutex_init(&monitor_lock);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index c5850e8..044bb47 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -9,6 +9,7 @@ stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
 stub-obj-y += cpu-get-icount.o
 stub-obj-y += dump.o
+stub-obj-y += error-printf.o
 stub-obj-y += fdset-add-fd.o
 stub-obj-y += fdset-find-fd.o
 stub-obj-y += fdset-get-fd.o
@@ -22,7 +23,6 @@ stub-obj-y += is-daemonized.o
 stub-obj-y += machine-init-done.o
 stub-obj-y += migr-blocker.o
 stub-obj-y += mon-is-qmp.o
-stub-obj-y += mon-printf.o
 stub-obj-y += monitor-init.o
 stub-obj-y += notify-event.o
 stub-obj-y += qtest.o
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
new file mode 100644
index 0000000..56379e6
--- /dev/null
+++ b/stubs/error-printf.c
@@ -0,0 +1,13 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/error-report.h"
+
+void error_vprintf(const char *fmt, va_list ap)
+{
+    vfprintf(stderr, fmt, ap);
+}
+
+void error_vprintf_unless_qmp(const char *fmt, va_list ap)
+{
+    error_vprintf(fmt, ap);
+}
diff --git a/stubs/mon-printf.c b/stubs/mon-printf.c
deleted file mode 100644
index e7c1e0c..0000000
--- a/stubs/mon-printf.c
+++ /dev/null
@@ -1,11 +0,0 @@
-#include "qemu/osdep.h"
-#include "qemu-common.h"
-#include "monitor/monitor.h"
-
-void monitor_printf(Monitor *mon, const char *fmt, ...)
-{
-}
-
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
-{
-}
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 1ef3566..b331f8f 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -14,24 +14,6 @@
 #include "monitor/monitor.h"
 #include "qemu/error-report.h"
 
-/*
- * Print to current monitor if we have one, else to stderr.
- * TODO should return int, so callers can calculate width, but that
- * requires surgery to monitor_vprintf().  Left for another day.
- */
-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);
-    }
-}
-
-/*
- * Print to current monitor if we have one, else to stderr.
- * TODO just like error_vprintf()
- */
 void error_printf(const char *fmt, ...)
 {
     va_list ap;
@@ -45,11 +27,9 @@ void error_printf_unless_qmp(const char *fmt, ...)
 {
     va_list ap;
 
-    if (!monitor_cur_is_qmp()) {
-        va_start(ap, fmt);
-        error_vprintf(fmt, ap);
-        va_end(ap);
-    }
+    va_start(ap, fmt);
+    error_vprintf_unless_qmp(fmt, ap);
+    va_end(ap);
 }
 
 static Location std_loc = {
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] tests: send error_report to test log

[Paolo Bonzini]

Implement error_vprintf to send the output of error_report to
the test log.  This silences test-vmstate.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/glib-compat.h | 13 +++++++++++++
 stubs/error-printf.c  |  8 +++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 8093163..9f86dc8 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -304,4 +304,17 @@ static inline void g_slist_free_full(GSList *list, GDestroyNotify free_func)
 }
 #endif
 
+#if !GLIB_CHECK_VERSION(2, 36, 0)
+/* Always fail.  This will not include error_report output in the test log,
+ * sending it instead to stderr.
+ */
+#define g_test_initialized() (0)
+#endif
+#if !GLIB_CHECK_VERSION(2, 38, 0)
+#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
+#error schizophrenic detection of glib subprocess testing
+#endif
+#define g_test_subprocess() (0)
+#endif
+
 #endif
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index 56379e6..ac6b92a 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -4,7 +4,13 @@
 
 void error_vprintf(const char *fmt, va_list ap)
 {
-    vfprintf(stderr, fmt, ap);
+    if (g_test_initialized() && !g_test_subprocess()) {
+        char *msg = g_strdup_vprintf(fmt, ap);
+        g_test_message("%s", msg);
+        g_free(msg);
+    } else {
+        vfprintf(stderr, fmt, ap);
+    }
 }
 
 void error_vprintf_unless_qmp(const char *fmt, va_list ap)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] tests: send error_report to test log

[Halil Pasic]

On 10/24/2016 06:31 PM, Paolo Bonzini wrote:
> Implement error_vprintf to send the output of error_report to
> the test log.  This silences test-vmstate.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[..]
>  void error_vprintf(const char *fmt, va_list ap)
>  {
> -    vfprintf(stderr, fmt, ap);
> +    if (g_test_initialized() && !g_test_subprocess()) {

I like the approach. What I do not like is:
* we still generate noise only less visible
* it's indiscriminate (silences all error reports of this type)
* can't be incorporated into the test logic

IMHO these are no major problems, and what I'm going to suggest
might be over-engineering, but I will ask nevertheless.

Have you considered a thread local function pointer to 
defaulting to something sane (eg. to this test log stuff
or to a wrapped vprintf)?

This would invalidate all the points above, and make the compat
macros unnecessary -- which have bitten me on my ancient RHEL
(because glib is too old). I have tried out the idea, so
if you want I can send you a patch on top of this.

Halil

> +        char *msg = g_strdup_vprintf(fmt, ap);
> +        g_test_message("%s", msg);
> +        g_free(msg);
> +    } else {
> +        vfprintf(stderr, fmt, ap);
> +    }
>  }
> 
>  void error_vprintf_unless_qmp(const char *fmt, va_list ap)
> 

Re: [Qemu-devel] [PATCH 2/2] tests: send error_report to test log

[Paolo Bonzini]

On 25/10/2016 13:06, Halil Pasic wrote:
> 
> 
> On 10/24/2016 06:31 PM, Paolo Bonzini wrote:
>> Implement error_vprintf to send the output of error_report to
>> the test log.  This silences test-vmstate.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [..]
>>  void error_vprintf(const char *fmt, va_list ap)
>>  {
>> -    vfprintf(stderr, fmt, ap);
>> +    if (g_test_initialized() && !g_test_subprocess()) {
> 
> I like the approach. What I do not like is:
> * we still generate noise only less visible

It's not noise if it ends up exactly in the right place (the test logs). :)

Paolo

> * it's indiscriminate (silences all error reports of this type)
> * can't be incorporated into the test logic
> 
> IMHO these are no major problems, and what I'm going to suggest
> might be over-engineering, but I will ask nevertheless.
> 
> Have you considered a thread local function pointer to 
> defaulting to something sane (eg. to this test log stuff
> or to a wrapped vprintf)?
> 
> This would invalidate all the points above, and make the compat
> macros unnecessary -- which have bitten me on my ancient RHEL
> (because glib is too old). I have tried out the idea, so
> if you want I can send you a patch on top of this.
> 
> Halil
> 
>> +        char *msg = g_strdup_vprintf(fmt, ap);
>> +        g_test_message("%s", msg);
>> +        g_free(msg);
>> +    } else {
>> +        vfprintf(stderr, fmt, ap);
>> +    }
>>  }
>>
>>  void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>>
> 

Re: [Qemu-devel] [PATCH 2/2] tests: send error_report to test log

[Halil Pasic]

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



On 10/25/2016 01:34 PM, Paolo Bonzini wrote:
> 
> On 25/10/2016 13:06, Halil Pasic wrote:
>> > 
>> > 
>> > On 10/24/2016 06:31 PM, Paolo Bonzini wrote:
>>> >> Implement error_vprintf to send the output of error_report to
>>> >> the test log.  This silences test-vmstate.
>>> >>
>>> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> > [..]
>>> >>  void error_vprintf(const char *fmt, va_list ap)
>>> >>  {
>>> >> -    vfprintf(stderr, fmt, ap);
>>> >> +    if (g_test_initialized() && !g_test_subprocess()) {
>> > 
>> > I like the approach. What I do not like is:
>> > * we still generate noise only less visible
> It's not noise if it ends up exactly in the right place (the test logs). :)
> 
> Paolo
> 

I assumed (for some strange reason) that the messages (submitted via
g_test_message) will not only appear in the xml report but also in the
html report. I was wrong. And while I still believe that the messages in
question (the vmstate test) do not add any value to the xml report and
on the other hand that situations could emerge where having the error
reported more prominently would be more convenient, I am very much fine
with this solution.

Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>


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

Re: [Qemu-devel] [PATCH 2/2] tests: send error_report to test log

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Implement error_vprintf to send the output of error_report to
> the test log.  This silences test-vmstate.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I think I'm OK with that, as long as people are happy with them
still getting stuff on stderr on older glib's.

The advantage over mine is that there's no per-test change.

Dave

> ---
>  include/glib-compat.h | 13 +++++++++++++
>  stubs/error-printf.c  |  8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 8093163..9f86dc8 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -304,4 +304,17 @@ static inline void g_slist_free_full(GSList *list, GDestroyNotify free_func)
>  }
>  #endif
>  
> +#if !GLIB_CHECK_VERSION(2, 36, 0)
> +/* Always fail.  This will not include error_report output in the test log,
> + * sending it instead to stderr.
> + */
> +#define g_test_initialized() (0)
> +#endif
> +#if !GLIB_CHECK_VERSION(2, 38, 0)
> +#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
> +#error schizophrenic detection of glib subprocess testing
> +#endif
> +#define g_test_subprocess() (0)
> +#endif
> +
>  #endif
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> index 56379e6..ac6b92a 100644
> --- a/stubs/error-printf.c
> +++ b/stubs/error-printf.c
> @@ -4,7 +4,13 @@
>  
>  void error_vprintf(const char *fmt, va_list ap)
>  {
> -    vfprintf(stderr, fmt, ap);
> +    if (g_test_initialized() && !g_test_subprocess()) {
> +        char *msg = g_strdup_vprintf(fmt, ap);
> +        g_test_message("%s", msg);
> +        g_free(msg);
> +    } else {
> +        vfprintf(stderr, fmt, ap);
> +    }
>  }
>  
>  void error_vprintf_unless_qmp(const char *fmt, va_list ap)
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK