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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jan 13, 2026 at 02:38:19PM +0100, Markus Armbruster wrote:
> 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?

There is only 1 stub after this patch - for monitor_vprintf().
error_vprintf() is never stubbed anymore.

The interesting scenario outside there tests is the non-system emulator
binaries.

Those will not have the monitor code, so will get the monitor_vprintf
stub. That will report g_test_initialized() == false, and so the stub
will return -1.  error_vprintf() will see this -1 return value and
thus call to vfprintf().

So the behaviour is again the same as before this patch AFAICT for all
linkage scenarios.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|