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

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

[Paolo Bonzini]

Leave the implementation of error_printf, error_printf_unless_qmp
and error_vprintf to libqemustub.a and monitor.c, so that we can
remove the monitor_printf and monitor_vprintf stubs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        This should help shutting up the vmstate unit tests.

 monitor.c            | 38 ++++++++++++++++++++++++++++++++++++++
 stubs/Makefile.objs  |  2 +-
 stubs/error-printf.c | 26 ++++++++++++++++++++++++++
 stubs/mon-printf.c   | 11 -----------
 util/qemu-error.c    | 38 --------------------------------------
 5 files changed, 65 insertions(+), 50 deletions(-)
 create mode 100644 stubs/error-printf.c
 delete mode 100644 stubs/mon-printf.c

diff --git a/monitor.c b/monitor.c
index b73a999..dab910f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3957,6 +3957,44 @@ 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);
+    }
+}
+
+/*
+ * 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;
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+}
+
+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);
+    }
+}
+
 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..19f7dd0
--- /dev/null
+++ b/stubs/error-printf.c
@@ -0,0 +1,26 @@
+#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_printf(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(ap);
+}
+
+void error_printf_unless_qmp(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vprintf(fmt, ap);
+    va_end(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..dffd781 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -14,44 +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;
-
-    va_start(ap, fmt);
-    error_vprintf(fmt, ap);
-    va_end(ap);
-}
-
-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);
-    }
-}
-
 static Location std_loc = {
     .kind = LOC_NONE
 };
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Leave the implementation of error_printf, error_printf_unless_qmp
> and error_vprintf to libqemustub.a and monitor.c, so that we can
> remove the monitor_printf and monitor_vprintf stubs.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         This should help shutting up the vmstate unit tests.

Why does this make it any easier than my patch?
You're still going to need to add something stub specific to turn
the output on and off.

Dave

>  monitor.c            | 38 ++++++++++++++++++++++++++++++++++++++
>  stubs/Makefile.objs  |  2 +-
>  stubs/error-printf.c | 26 ++++++++++++++++++++++++++
>  stubs/mon-printf.c   | 11 -----------
>  util/qemu-error.c    | 38 --------------------------------------
>  5 files changed, 65 insertions(+), 50 deletions(-)
>  create mode 100644 stubs/error-printf.c
>  delete mode 100644 stubs/mon-printf.c
> 
> diff --git a/monitor.c b/monitor.c
> index b73a999..dab910f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3957,6 +3957,44 @@ 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);
> +    }
> +}
> +
> +/*
> + * 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;
> +
> +    va_start(ap, fmt);
> +    error_vprintf(fmt, ap);
> +    va_end(ap);
> +}
> +
> +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);
> +    }
> +}
> +
>  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..19f7dd0
> --- /dev/null
> +++ b/stubs/error-printf.c
> @@ -0,0 +1,26 @@
> +#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_printf(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    error_vprintf(fmt, ap);
> +    va_end(ap);
> +}
> +
> +void error_printf_unless_qmp(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    error_vprintf(fmt, ap);
> +    va_end(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..dffd781 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -14,44 +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;
> -
> -    va_start(ap, fmt);
> -    error_vprintf(fmt, ap);
> -    va_end(ap);
> -}
> -
> -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);
> -    }
> -}
> -
>  static Location std_loc = {
>      .kind = LOC_NONE
>  };
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Paolo Bonzini]

On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> Leave the implementation of error_printf, error_printf_unless_qmp
>> and error_vprintf to libqemustub.a and monitor.c, so that we can
>> remove the monitor_printf and monitor_vprintf stubs.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>         This should help shutting up the vmstate unit tests.
> 
> Why does this make it any easier than my patch?
> You're still going to need to add something stub specific to turn
> the output on and off.

It makes it possible to override the functions independent of the rest
of util/qemu-error.c. You can implement the functions in the test, simply as

    had_stderr_output = true;

and then assert that had_stderr_output is false or true depending on the
test.

(It's also a useful starting point to fix the cur_mon race).

Consider this an RFC.  error_printf probably should stay in qemu-error.c
since it can always call error_vprintf.

Paolo

> Dave
> 
>>  monitor.c            | 38 ++++++++++++++++++++++++++++++++++++++
>>  stubs/Makefile.objs  |  2 +-
>>  stubs/error-printf.c | 26 ++++++++++++++++++++++++++
>>  stubs/mon-printf.c   | 11 -----------
>>  util/qemu-error.c    | 38 --------------------------------------
>>  5 files changed, 65 insertions(+), 50 deletions(-)
>>  create mode 100644 stubs/error-printf.c
>>  delete mode 100644 stubs/mon-printf.c
>>
>> diff --git a/monitor.c b/monitor.c
>> index b73a999..dab910f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3957,6 +3957,44 @@ 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);
>> +    }
>> +}
>> +
>> +/*
>> + * 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;
>> +
>> +    va_start(ap, fmt);
>> +    error_vprintf(fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>> +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);
>> +    }
>> +}
>> +
>>  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..19f7dd0
>> --- /dev/null
>> +++ b/stubs/error-printf.c
>> @@ -0,0 +1,26 @@
>> +#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_printf(const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    error_vprintf(fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>> +void error_printf_unless_qmp(const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    error_vprintf(fmt, ap);
>> +    va_end(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..dffd781 100644
>> --- a/util/qemu-error.c
>> +++ b/util/qemu-error.c
>> @@ -14,44 +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;
>> -
>> -    va_start(ap, fmt);
>> -    error_vprintf(fmt, ap);
>> -    va_end(ap);
>> -}
>> -
>> -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);
>> -    }
>> -}
>> -
>>  static Location std_loc = {
>>      .kind = LOC_NONE
>>  };
>> -- 
>> 2.7.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>> Leave the implementation of error_printf, error_printf_unless_qmp
>>> and error_vprintf to libqemustub.a and monitor.c, so that we can
>>> remove the monitor_printf and monitor_vprintf stubs.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>         This should help shutting up the vmstate unit tests.
>> 
>> Why does this make it any easier than my patch?
>
>> You're still going to need to add something stub specific to turn
>> the output on and off.
>
> It makes it possible to override the functions independent of the rest
> of util/qemu-error.c. You can implement the functions in the test, simply as
>
>     had_stderr_output = true;
>
> and then assert that had_stderr_output is false or true depending on the
> test.

I buy that when I see a test using it :)

> (It's also a useful starting point to fix the cur_mon race).

Uh, the fix for the cur_mon race is making it thread-local, isn't it?

> Consider this an RFC.  error_printf probably should stay in qemu-error.c
> since it can always call error_vprintf.
>
> Paolo
>
>> Dave
>> 
>>>  monitor.c            | 38 ++++++++++++++++++++++++++++++++++++++
>>>  stubs/Makefile.objs  |  2 +-
>>>  stubs/error-printf.c | 26 ++++++++++++++++++++++++++
>>>  stubs/mon-printf.c   | 11 -----------
>>>  util/qemu-error.c    | 38 --------------------------------------
>>>  5 files changed, 65 insertions(+), 50 deletions(-)
>>>  create mode 100644 stubs/error-printf.c
>>>  delete mode 100644 stubs/mon-printf.c
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index b73a999..dab910f 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -3957,6 +3957,44 @@ 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);
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * 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;
>>> +
>>> +    va_start(ap, fmt);
>>> +    error_vprintf(fmt, ap);
>>> +    va_end(ap);
>>> +}
>>> +
>>> +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);
>>> +    }
>>> +}
>>> +
>>>  static void __attribute__((constructor)) monitor_lock_init(void)
>>>  {
>>>      qemu_mutex_init(&monitor_lock);

monitor.c has >3400 SLOC.  I'd consider a separate error-printf.c.

>>> 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..19f7dd0
>>> --- /dev/null
>>> +++ b/stubs/error-printf.c
>>> @@ -0,0 +1,26 @@
>>> +#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_printf(const char *fmt, ...)
>>> +{
>>> +    va_list ap;
>>> +
>>> +    va_start(ap, fmt);
>>> +    error_vprintf(fmt, ap);
>>> +    va_end(ap);
>>> +}
>>> +
>>> +void error_printf_unless_qmp(const char *fmt, ...)
>>> +{
>>> +    va_list ap;
>>> +
>>> +    va_start(ap, fmt);
>>> +    error_vprintf(fmt, ap);
>>> +    va_end(ap);
>>> +}

Copy of the non-stub code partially evaluated for !cur_mon &&
!monitor_cur_is_qmp().  Okay if the copy earns its keep.  It can't until
it has actual users :)

>>> 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..dffd781 100644
>>> --- a/util/qemu-error.c
>>> +++ b/util/qemu-error.c
>>> @@ -14,44 +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;
>>> -
>>> -    va_start(ap, fmt);
>>> -    error_vprintf(fmt, ap);
>>> -    va_end(ap);
>>> -}
>>> -
>>> -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);
>>> -    }
>>> -}
>>> -
>>>  static Location std_loc = {
>>>      .kind = LOC_NONE
>>>  };
>>> -- 
>>> 2.7.4
>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Paolo Bonzini]

On 24/10/2016 15:08, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>>> Leave the implementation of error_printf, error_printf_unless_qmp
>>>> and error_vprintf to libqemustub.a and monitor.c, so that we can
>>>> remove the monitor_printf and monitor_vprintf stubs.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>         This should help shutting up the vmstate unit tests.
>>>
>>> Why does this make it any easier than my patch?
>>
>>> You're still going to need to add something stub specific to turn
>>> the output on and off.
>>
>> It makes it possible to override the functions independent of the rest
>> of util/qemu-error.c. You can implement the functions in the test, simply as
>>
>>     had_stderr_output = true;
>>
>> and then assert that had_stderr_output is false or true depending on the
>> test.
> 
> I buy that when I see a test using it :)

Ok, so should I rewrite the test-vmstate patch to do this?

>> (It's also a useful starting point to fix the cur_mon race).
> 
> Uh, the fix for the cur_mon race is making it thread-local, isn't it?

Or just old-school mutex.  There is monitor_lock, let's make it protect
cur_mon.

Paolo

>> Consider this an RFC.  error_printf probably should stay in qemu-error.c
>> since it can always call error_vprintf.
>>
>> Paolo
>>
>>> Dave
>>>
>>>>  monitor.c            | 38 ++++++++++++++++++++++++++++++++++++++
>>>>  stubs/Makefile.objs  |  2 +-
>>>>  stubs/error-printf.c | 26 ++++++++++++++++++++++++++
>>>>  stubs/mon-printf.c   | 11 -----------
>>>>  util/qemu-error.c    | 38 --------------------------------------
>>>>  5 files changed, 65 insertions(+), 50 deletions(-)
>>>>  create mode 100644 stubs/error-printf.c
>>>>  delete mode 100644 stubs/mon-printf.c
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index b73a999..dab910f 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -3957,6 +3957,44 @@ 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);
>>>> +    }
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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;
>>>> +
>>>> +    va_start(ap, fmt);
>>>> +    error_vprintf(fmt, ap);
>>>> +    va_end(ap);
>>>> +}
>>>> +
>>>> +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);
>>>> +    }
>>>> +}
>>>> +
>>>>  static void __attribute__((constructor)) monitor_lock_init(void)
>>>>  {
>>>>      qemu_mutex_init(&monitor_lock);
> 
> monitor.c has >3400 SLOC.  I'd consider a separate error-printf.c.
> 
>>>> 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..19f7dd0
>>>> --- /dev/null
>>>> +++ b/stubs/error-printf.c
>>>> @@ -0,0 +1,26 @@
>>>> +#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_printf(const char *fmt, ...)
>>>> +{
>>>> +    va_list ap;
>>>> +
>>>> +    va_start(ap, fmt);
>>>> +    error_vprintf(fmt, ap);
>>>> +    va_end(ap);
>>>> +}
>>>> +
>>>> +void error_printf_unless_qmp(const char *fmt, ...)
>>>> +{
>>>> +    va_list ap;
>>>> +
>>>> +    va_start(ap, fmt);
>>>> +    error_vprintf(fmt, ap);
>>>> +    va_end(ap);
>>>> +}
> 
> Copy of the non-stub code partially evaluated for !cur_mon &&
> !monitor_cur_is_qmp().  Okay if the copy earns its keep.  It can't until
> it has actual users :)
> 
>>>> 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..dffd781 100644
>>>> --- a/util/qemu-error.c
>>>> +++ b/util/qemu-error.c
>>>> @@ -14,44 +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;
>>>> -
>>>> -    va_start(ap, fmt);
>>>> -    error_vprintf(fmt, ap);
>>>> -    va_end(ap);
>>>> -}
>>>> -
>>>> -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);
>>>> -    }
>>>> -}
>>>> -
>>>>  static Location std_loc = {
>>>>      .kind = LOC_NONE
>>>>  };
>>>> -- 
>>>> 2.7.4
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/10/2016 15:08, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>>>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>>>> Leave the implementation of error_printf, error_printf_unless_qmp
>>>>> and error_vprintf to libqemustub.a and monitor.c, so that we can
>>>>> remove the monitor_printf and monitor_vprintf stubs.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> ---
>>>>>         This should help shutting up the vmstate unit tests.
>>>>
>>>> Why does this make it any easier than my patch?
>>>
>>>> You're still going to need to add something stub specific to turn
>>>> the output on and off.
>>>
>>> It makes it possible to override the functions independent of the rest
>>> of util/qemu-error.c. You can implement the functions in the test, simply as
>>>
>>>     had_stderr_output = true;
>>>
>>> and then assert that had_stderr_output is false or true depending on the
>>> test.
>> 
>> I buy that when I see a test using it :)
>
> Ok, so should I rewrite the test-vmstate patch to do this?

You need to rewrite an existing test or create a new one to demonstrate
your approach is worthwhile.  Details are up to you.

>>> (It's also a useful starting point to fix the cur_mon race).
>> 
>> Uh, the fix for the cur_mon race is making it thread-local, isn't it?
>
> Or just old-school mutex.  There is monitor_lock, let's make it protect
> cur_mon.

cur_mon is semantically thread-local: it's non-null while we're
executing a monitor command.  That's a property of the stack, thus the
thread.  The fact that it's not actually thread-local now is a bug I
blame on inertia.

Further evidence: if a thread calls error_report(), it should honor
cur_mon *only* when *this* thread executes a monitor command.  It should
not spew to some unrelated monitor just because some other thread
happens to execute a monitor command right now.

monitor_lock is different: it's for protecting data that's *shared*
among monitors, such as mon_list.  I suspect it's not quite used that
way.  But it should.

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Paolo Bonzini]

On 24/10/2016 16:19, Markus Armbruster wrote:
> cur_mon is semantically thread-local: it's non-null while we're
> executing a monitor command.  That's a property of the stack, thus the
> thread.  The fact that it's not actually thread-local now is a bug I
> blame on inertia.

Or just not noticing it until now.

> Further evidence: if a thread calls error_report(), it should honor
> cur_mon *only* when *this* thread executes a monitor command.  It should
> not spew to some unrelated monitor just because some other thread
> happens to execute a monitor command right now.

Yes, that makes sense.  I'll work on redoing David's patch on top of
this one then.

Paolo

Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Halil Pasic]

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



On 10/24/2016 01:52 PM, Paolo Bonzini wrote:
> 
> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>> >> Leave the implementation of error_printf, error_printf_unless_qmp
>>> >> and error_vprintf to libqemustub.a and monitor.c, so that we can
>>> >> remove the monitor_printf and monitor_vprintf stubs.
>>> >>
>>> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> >> ---
>>> >>         This should help shutting up the vmstate unit tests.
>> > 
>> > Why does this make it any easier than my patch?
>> > You're still going to need to add something stub specific to turn
>> > the output on and off.
> It makes it possible to override the functions independent of the rest
> of util/qemu-error.c. You can implement the functions in the test, simply as
> 
>     had_stderr_output = true;
> 
> and then assert that had_stderr_output is false or true depending on the
> test.
> 
> (It's also a useful starting point to fix the cur_mon race).
> 
> Consider this an RFC.  error_printf probably should stay in qemu-error.c
> since it can always call error_vprintf.
> 
> Paolo
> 

Agree with Paolo that the ability to provide a custom (test specific)
implementation is generally beneficial testing and IMHO superior
compared to the solution previously proposed by Dave. Unfortunately I do
not see how is this accomplished by the patch.

I still consider using Error objects in the code under test and
not reporting errors directly the cleanest approach.

Halil


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