On 24/10/2016 15:08, Markus Armbruster wrote:
> 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 :)
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 / dgilb...@redhat.com / Manchester, UK
>>>
