Paolo Bonzini <pbonz...@redhat.com> writes:
> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (pbonz...@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 <pbonz...@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 / dgilb...@redhat.com / Manchester, UK
>>
