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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (address@hidden) 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 <address@hidden>
>>> ---
>>>         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 / address@hidden / Manchester, UK
>>