Re: [PATCH v5 14/24] monitor: move error_vprintf back to error-report.c

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The current unit tests rely on monitor.o not being linked, such
> that the monitor stubs get linked instead. Since error_vprintf
> is in monitor.o this allows a stub error_vprintf impl to be used
> that calls g_test_message.
>
> This takes a different approach, with error_vprintf moving
> back to error-report.c such that it is always linked into the
> tests. The monitor_vprintf() stub is then changed to use
> g_test_message if QTEST_SILENT_ERRORS is set, otherwise it will
> return -1 and trigger error_vprintf to call vfprintf.
>
> The end result is functionally equivalent for the purposes of
> the unit tests.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  monitor/monitor.c    | 15 ---------------
>  stubs/error-printf.c | 18 ------------------
>  stubs/meson.build    |  1 -
>  stubs/monitor-core.c | 14 +++++++++++++-
>  util/error-report.c  | 15 +++++++++++++++
>  5 files changed, 28 insertions(+), 35 deletions(-)
>  delete mode 100644 stubs/error-printf.c
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 627a59b23e..6dc5a7016d 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -268,21 +268,6 @@ void monitor_printc(Monitor *mon, int c)
>      monitor_printf(mon, "'");
>  }
>  
> -int error_vprintf(const char *fmt, va_list ap)
> -{
> -    Monitor *cur_mon = monitor_cur();
> -    /*
> -     * This will return -1 if 'cur_mon' is NULL, or is QMP.
> -     * IOW this will only print if in HMP, otherwise we
> -     * fallback to stderr for QMP / no-monitor scenarios.
> -     */
> -    int ret = monitor_vprintf(cur_mon, fmt, ap);
> -    if (ret == -1) {
> -        ret = vfprintf(stderr, fmt, ap);
> -    }
> -    return ret;
> -}
> -
>  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      /* Limit guest-triggerable events to 1 per second */
>      [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> deleted file mode 100644
> index 1afa0f62ca..0000000000
> --- a/stubs/error-printf.c
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -#include "qemu/osdep.h"
> -#include "qemu/error-report.h"
> -#include "monitor/monitor.h"
> -
> -int error_vprintf(const char *fmt, va_list ap)
> -{
> -    int ret;
> -
> -    if (g_test_initialized() && !g_test_subprocess() &&
> -        getenv("QTEST_SILENT_ERRORS")) {
> -        char *msg = g_strdup_vprintf(fmt, ap);
> -        g_test_message("%s", msg);
> -        ret = strlen(msg);
> -        g_free(msg);
> -        return ret;
> -    }
> -    return vfprintf(stderr, fmt, ap);
> -}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 0b2778c568..3d77458a3f 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -3,7 +3,6 @@
>  # below, so that it is clear who needs the stubbed functionality.
>  
>  stub_ss.add(files('cpu-get-clock.c'))
> -stub_ss.add(files('error-printf.c'))
>  stub_ss.add(files('fdset.c'))
>  stub_ss.add(files('iothread-lock.c'))
>  stub_ss.add(files('is-daemonized.c'))
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index 1894cdfe1f..a7c32297c9 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -18,5 +18,17 @@ void qapi_event_emit(QAPIEvent event, QDict *qdict)
>  
>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
> -    abort();
> +    /*
> +     * Pretend 'g_test_message' is our monitor console to
> +     * stop the caller sending messages to stderr
> +     */
> +    if (g_test_initialized() && !g_test_subprocess() &&
> +        getenv("QTEST_SILENT_ERRORS")) {
> +        char *msg = g_strdup_vprintf(fmt, ap);
> +        g_test_message("%s", msg);
> +        size_t ret = strlen(msg);
> +        g_free(msg);
> +        return ret;
> +    }
> +    return -1;
>  }
> diff --git a/util/error-report.c b/util/error-report.c
> index 1b17c11de1..b262ad01cb 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -29,6 +29,21 @@ bool message_with_timestamp;
>  bool error_with_guestname;
>  const char *error_guest_name;
>  
> +int error_vprintf(const char *fmt, va_list ap)
> +{
> +    Monitor *cur_mon = monitor_cur();
> +    /*
> +     * This will return -1 if 'cur_mon' is NULL, or is QMP.
> +     * IOW this will only print if in HMP, otherwise we
> +     * fallback to stderr for QMP / no-monitor scenarios.
> +     */
> +    int ret = monitor_vprintf(cur_mon, fmt, ap);
> +    if (ret == -1) {
> +        ret = vfprintf(stderr, fmt, ap);
> +    }
> +    return ret;
> +}
> +
>  int error_printf(const char *fmt, ...)
>  {
>      va_list ap;

Without stubs, no change in behavior.

With both stubs, before the patch:

    monitor_vprintf() is not supposed to run, and aborts

    error_vprintf() calls g_test_message() for tests, else vfprintf()

afterwards:

    monitor_vprintf() calls g_test_message() and succeeds in tests, else
    fails

    error_vprintf() calls monitor_printf(), and when it fails falls back
    to vfprintf().

Alright, error_vprintf() behaves the same as before.

monitor_vprintf() no longer aborts.  Hmm.  What if we somehow acquire
calls?  In tests, they'll go to g_test_message(), which is fine, I
guess.  Outside tests, they'll fail.  So does the non-stub version
unless the current monitor is HMP.  Also fine, I guess.

Is it possible to link just one of the stubs?