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

[Paolo Bonzini <pbonzini@redhat.com>]

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
>>>