[RFC 0/4] util: qmessage_context followup

[RFC 0/4] util: qmessage_context followup

[Richard Henderson]

Hi Daniel,

I'm still not keen on qmessage_context allocating a string.

If we *did* allocate a string, it should be a GString so that we can
easily append to that.  The only benefit I see from this is that we
would collect all of the output and it would reach stderr atomically.

However, I think it's better to not collect the message and just
output the pieces.  Something like the following.  The names are
horrible and I didn't document the patches well.


r~


Richard Henderson (4):
  util: Introduce LogOutput
  util: Drop QMESSAGE_CONTEXT_SKIP_MONITOR
  util/message: Use LogOutput
  util/error-report: Use LogOutput in vreport

 include/monitor/monitor.h |  4 +++
 include/qemu/log-output.h | 14 ++++++++
 include/qemu/message.h    | 21 +++--------
 monitor/monitor.c         | 54 +++++++++++++++++++++++------
 stubs/error-printf.c      | 11 ++++++
 util/error-report.c       | 73 +++++++++++++++++++--------------------
 util/log.c                | 23 +++++-------
 util/message.c            | 45 ++++++++++--------------
 8 files changed, 139 insertions(+), 106 deletions(-)
 create mode 100644 include/qemu/log-output.h

-- 
2.43.0

[RFC 1/4] util: Introduce LogOutput

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/monitor/monitor.h |  4 +++
 include/qemu/log-output.h | 14 ++++++++++
 monitor/monitor.c         | 54 +++++++++++++++++++++++++++++++--------
 stubs/error-printf.c      | 11 ++++++++
 util/log.c                |  7 +++++
 5 files changed, 79 insertions(+), 11 deletions(-)
 create mode 100644 include/qemu/log-output.h

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c3740ec616..8c4d73592f 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -4,6 +4,7 @@
 #include "block/block.h"
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
+#include "qemu/log-output.h"
 #include "exec/hwaddr.h"
 
 typedef struct MonitorHMP MonitorHMP;
@@ -62,4 +63,7 @@ void monitor_register_hmp_info_hrt(const char *name,
 int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
 int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
 
+const LogOutput *error_log_output(void **popaque);
+const LogOutput *error_log_output_unless_qmp(void **popaque);
+
 #endif /* MONITOR_H */
diff --git a/include/qemu/log-output.h b/include/qemu/log-output.h
new file mode 100644
index 0000000000..1d502aae77
--- /dev/null
+++ b/include/qemu/log-output.h
@@ -0,0 +1,14 @@
+#ifndef QEMU_LOG_OUTPUT_H
+#define QEMU_LOG_OUTPUT_H 1
+
+typedef int LogOutputVararg(void *, const char *, ...) G_GNUC_PRINTF(2, 3);
+typedef int LogOutputVaList(void *, const char *, va_list) G_GNUC_PRINTF(2, 0);
+
+typedef struct LogOutput {
+    LogOutputVararg *print;
+    LogOutputVaList *vprint;
+} LogOutput;
+
+extern const LogOutput log_output_stdio;
+
+#endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index da54e1b1ce..71a3d62e0f 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -268,28 +268,60 @@ void monitor_printc(Monitor *mon, int c)
     monitor_printf(mon, "'");
 }
 
+static const LogOutput log_output_monitor = {
+    /* Abuse the Monitor* argument of monitor_printf as void* opaque. */
+    (LogOutputVararg *)monitor_printf,
+    (LogOutputVaList *)monitor_vprintf,
+};
+
+const LogOutput *error_log_output(void **popaque)
+{
+    Monitor *cur_mon = monitor_cur();
+
+    if (cur_mon && !monitor_cur_is_qmp()) {
+        *popaque = cur_mon;
+        return &log_output_monitor;
+    }
+
+    *popaque = stderr;
+    return &log_output_stdio;
+}
+
+const LogOutput *error_log_output_unless_qmp(void **popaque)
+{
+    Monitor *cur_mon = monitor_cur();
+
+    if (!cur_mon) {
+        *popaque = stderr;
+        return &log_output_stdio;
+    }
+    if (!monitor_cur_is_qmp()) {
+        *popaque = cur_mon;
+        return &log_output_monitor;
+    }
+
+    *popaque = NULL;
+    return NULL;
+}
+
 /*
  * Print to current monitor if we have one, else to stderr.
  */
 int error_vprintf(const char *fmt, va_list ap)
 {
-    Monitor *cur_mon = monitor_cur();
+    void *opaque;
+    const LogOutput *l = error_log_output(&opaque);
 
-    if (cur_mon && !monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
-    }
-    return vfprintf(stderr, fmt, ap);
+    return l->vprint(opaque, fmt, ap);
 }
 
 int error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
-    Monitor *cur_mon = monitor_cur();
+    void *opaque;
+    const LogOutput *l = error_log_output_unless_qmp(&opaque);
 
-    if (!cur_mon) {
-        return vfprintf(stderr, fmt, ap);
-    }
-    if (!monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
+    if (l) {
+        return l->vprint(opaque, fmt, ap);
     }
     return -1;
 }
diff --git a/stubs/error-printf.c b/stubs/error-printf.c
index 0e326d8010..82e4756bda 100644
--- a/stubs/error-printf.c
+++ b/stubs/error-printf.c
@@ -21,3 +21,14 @@ int error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
     return error_vprintf(fmt, ap);
 }
+
+const LogOutput *error_log_output(void **popaque)
+{
+    *popaque = stderr;
+    return &log_output_stdio;
+}
+
+const LogOutput *error_log_output_unless_qmp(void **popaque)
+{
+    return error_log_output(popaque);
+}
diff --git a/util/log.c b/util/log.c
index fc900cde0c..4b5953dcc7 100644
--- a/util/log.c
+++ b/util/log.c
@@ -28,6 +28,7 @@
 #include "qemu/thread.h"
 #include "qemu/lockable.h"
 #include "qemu/rcu.h"
+#include "qemu/log-output.h"
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
 #endif
@@ -592,3 +593,9 @@ ssize_t rust_fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream)
     return ret < 0 ? -errno : 0;
 }
 #endif
+
+const LogOutput log_output_stdio = {
+    /* Abuse the FILE* argument of fprintf as void* opaque. */
+    (LogOutputVararg *)fprintf,
+    (LogOutputVaList *)vfprintf,
+};
-- 
2.43.0

[RFC 2/4] util: Drop QMESSAGE_CONTEXT_SKIP_MONITOR

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/message.h | 11 +----------
 util/error-report.c    |  9 +++++----
 util/log.c             |  2 +-
 util/message.c         |  7 +------
 4 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/include/qemu/message.h b/include/qemu/message.h
index 6dbb068ca9..68b08f0ea7 100644
--- a/include/qemu/message.h
+++ b/include/qemu/message.h
@@ -28,25 +28,16 @@ void qmessage_set_format(int flags);
  */
 void qmessage_set_workload_name(const char *name);
 
-enum QMessageContextFlags {
-    QMESSAGE_CONTEXT_SKIP_MONITOR = (1 << 0),
-};
-
 /**
  * qmessage_context:
- * @flags: the message formatting control flags
  *
  * Format a message prefix with the information
  * previously selected by a call to
  * qmessage_set_format.
  *
- * If @flags contains QMESSAGE_CONTEXT_SKIP_MONITOR
- * an empty string will be returned if running in
- * the context of a HMP command
- *
  * Returns: a formatted message prefix, or empty string;
  * to be freed by the caller.
  */
-char *qmessage_context(int flags);
+char *qmessage_context(void);
 
 #endif /* QEMU_MESSAGE_H */
diff --git a/util/error-report.c b/util/error-report.c
index 2e58ee1c50..fa34019dad 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -172,10 +172,11 @@ static void print_loc(void)
 G_GNUC_PRINTF(2, 0)
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
-    g_autofree gchar *context = qmessage_context(QMESSAGE_CONTEXT_SKIP_MONITOR);
-
-    if (context != NULL) {
-        error_printf("%s", context);
+    if (!monitor_cur()) {
+        g_autofree gchar *context = qmessage_context();
+        if (context) {
+            error_printf("%s", context);
+        }
     }
 
     print_loc();
diff --git a/util/log.c b/util/log.c
index 4b5953dcc7..b129634708 100644
--- a/util/log.c
+++ b/util/log.c
@@ -161,7 +161,7 @@ void qemu_log(const char *fmt, ...)
      * acquiring the mutex
      */
     g_autofree const char *context =
-        incomplete ? NULL : qmessage_context(0);
+        incomplete ? NULL : qmessage_context();
 
     f = qemu_log_trylock();
     if (f) {
diff --git a/util/message.c b/util/message.c
index 6d3580b7be..8deba3940c 100644
--- a/util/message.c
+++ b/util/message.c
@@ -20,17 +20,12 @@ void qmessage_set_workload_name(const char *name)
 }
 
 
-char *qmessage_context(int flags)
+char *qmessage_context(void)
 {
     g_autofree char *timestr = NULL;
     const char *wknamestr = NULL;
     const char *pgnamestr = NULL;
 
-    if ((flags & QMESSAGE_CONTEXT_SKIP_MONITOR) &&
-        monitor_cur()) {
-        return NULL;
-    }
-
     if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
         g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
         timestr = g_date_time_format_iso8601(dt);
-- 
2.43.0

[RFC 3/4] util/message: Use LogOutput

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/message.h | 12 +++++-------
 util/error-report.c    |  8 ++++----
 util/log.c             | 16 ++--------------
 util/message.c         | 40 ++++++++++++++++++----------------------
 4 files changed, 29 insertions(+), 47 deletions(-)

diff --git a/include/qemu/message.h b/include/qemu/message.h
index 68b08f0ea7..ac285a0021 100644
--- a/include/qemu/message.h
+++ b/include/qemu/message.h
@@ -3,6 +3,8 @@
 #ifndef QEMU_MESSAGE_H
 #define QEMU_MESSAGE_H
 
+#include "qemu/log-output.h"
+
 enum QMessageFormatFlags {
     QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
     QMESSAGE_FORMAT_WORKLOAD_NAME = (1 << 1),
@@ -31,13 +33,9 @@ void qmessage_set_workload_name(const char *name);
 /**
  * qmessage_context:
  *
- * Format a message prefix with the information
- * previously selected by a call to
- * qmessage_set_format.
- *
- * Returns: a formatted message prefix, or empty string;
- * to be freed by the caller.
+ * Format a message prefix with the information previously selected
+ * by a call to qmessage_set_format.
  */
-char *qmessage_context(void);
+void qmessage_context(const LogOutput *l, void *opaque);
 
 #endif /* QEMU_MESSAGE_H */
diff --git a/util/error-report.c b/util/error-report.c
index fa34019dad..6ef556af5f 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -173,10 +173,10 @@ G_GNUC_PRINTF(2, 0)
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
     if (!monitor_cur()) {
-        g_autofree gchar *context = qmessage_context();
-        if (context) {
-            error_printf("%s", context);
-        }
+        void *opaque;
+        const LogOutput *l = error_log_output(&opaque);
+
+        qmessage_context(l, opaque);
     }
 
     print_loc();
diff --git a/util/log.c b/util/log.c
index b129634708..8bbd8e5dda 100644
--- a/util/log.c
+++ b/util/log.c
@@ -153,23 +153,11 @@ static __thread bool incomplete;
 
 void qemu_log(const char *fmt, ...)
 {
-    FILE *f;
-    /*
-     * Prepare the context *outside* the logging
-     * lock so any timestamp better reflects when
-     * the message was emitted if we are delayed
-     * acquiring the mutex
-     */
-    g_autofree const char *context =
-        incomplete ? NULL : qmessage_context();
-
-    f = qemu_log_trylock();
+    FILE *f = qemu_log_trylock();
     if (f) {
         va_list ap;
 
-        if (context != NULL) {
-            fwrite(context, 1, strlen(context), f);
-        }
+        qmessage_context(&log_output_stdio, f);
 
         va_start(ap, fmt);
         vfprintf(f, fmt, ap);
diff --git a/util/message.c b/util/message.c
index 8deba3940c..0c63d128fc 100644
--- a/util/message.c
+++ b/util/message.c
@@ -20,7 +20,7 @@ void qmessage_set_workload_name(const char *name)
 }
 
 
-char *qmessage_context(void)
+void qmessage_context(const LogOutput *l, void *opaque)
 {
     g_autofree char *timestr = NULL;
     const char *wknamestr = NULL;
@@ -43,26 +43,22 @@ char *qmessage_context(void)
         int thid = qemu_get_thread_id();
         const char *thname = qemu_thread_get_name();
 
-        return g_strdup_printf("%s%s%s%s%s%s(%d:%s): ",
-                               timestr ? timestr : "",
-                               timestr ? " " : "",
-                               wknamestr ? "[" : "",
-                               wknamestr ? wknamestr : "",
-                               wknamestr ? "] " : "",
-                               pgnamestr ? pgnamestr : "",
-                               thid, thname);
-    } else {
-        if (!timestr && !wknamestr && !pgnamestr) {
-            return NULL;
-        }
-
-        return g_strdup_printf("%s%s%s%s%s%s%s",
-                               timestr ? timestr : "",
-                               timestr ? " " : "",
-                               wknamestr ? "[" : "",
-                               wknamestr ? wknamestr : "",
-                               wknamestr ? "] " : "",
-                               pgnamestr ? pgnamestr : "",
-                               pgnamestr ? ": " : "");
+        l->print(opaque, "%s%s%s%s%s%s(%d:%s): ",
+                 timestr ? timestr : "",
+                 timestr ? " " : "",
+                 wknamestr ? "[" : "",
+                 wknamestr ? wknamestr : "",
+                 wknamestr ? "] " : "",
+                 pgnamestr ? pgnamestr : "",
+                 thid, thname);
+    } else if (timestr || wknamestr || pgnamestr) {
+        l->print(opaque, "%s%s%s%s%s%s%s",
+                 timestr ? timestr : "",
+                 timestr ? " " : "",
+                 wknamestr ? "[" : "",
+                 wknamestr ? wknamestr : "",
+                 wknamestr ? "] " : "",
+                 pgnamestr ? pgnamestr : "",
+                 pgnamestr ? ": " : "");
     }
 }
-- 
2.43.0

[RFC 4/4] util/error-report: Use LogOutput in vreport

[Richard Henderson]

Merge print_loc.  Use the LogOutput callbacks instead of
error_printf and error_vprintf.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 util/error-report.c | 72 +++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 38 deletions(-)

diff --git a/util/error-report.c b/util/error-report.c
index 6ef556af5f..db5b2c1608 100644
--- a/util/error-report.c
+++ b/util/error-report.c
@@ -132,36 +132,6 @@ void loc_set_file(const char *fname, int lno)
     }
 }
 
-/*
- * Print current location to current monitor if we have one, else to stderr.
- */
-static void print_loc(void)
-{
-    const char *sep = "";
-    int i;
-    const char *const *argp;
-
-    switch (cur_loc->kind) {
-    case LOC_CMDLINE:
-        argp = cur_loc->ptr;
-        for (i = 0; i < cur_loc->num; i++) {
-            error_printf("%s%s", sep, argp[i]);
-            sep = " ";
-        }
-        error_printf(": ");
-        break;
-    case LOC_FILE:
-        error_printf("%s:", (const char *)cur_loc->ptr);
-        if (cur_loc->num) {
-            error_printf("%d:", cur_loc->num);
-        }
-        error_printf(" ");
-        break;
-    default:
-        error_printf("%s", sep);
-    }
-}
-
 /*
  * Print a message to current monitor if we have one, else to stderr.
  * @report_type is the type of message: error, warning or informational.
@@ -172,28 +142,54 @@ static void print_loc(void)
 G_GNUC_PRINTF(2, 0)
 static void vreport(report_type type, const char *fmt, va_list ap)
 {
-    if (!monitor_cur()) {
-        void *opaque;
-        const LogOutput *l = error_log_output(&opaque);
+    void *opaque;
+    const LogOutput *l = error_log_output(&opaque);
 
+    if (!monitor_cur()) {
         qmessage_context(l, opaque);
     }
 
-    print_loc();
+    switch (cur_loc->kind) {
+    case LOC_CMDLINE:
+        {
+            const char *const *argp = cur_loc->ptr;
+            const char *sep = "";
+
+            for (int i = 0; i < cur_loc->num; i++) {
+                l->print(opaque, "%s%s", sep, argp[i]);
+                sep = " ";
+            }
+            l->print(opaque, ": ");
+        }
+        break;
+    case LOC_FILE:
+        {
+            const char *file = cur_loc->ptr;
+
+            if (cur_loc->num) {
+                error_printf("%s:%d: ", file, cur_loc->num);
+            } else {
+                error_printf("%s: ", file);
+            }
+        }
+        break;
+    case LOC_NONE:
+        break;
+    }
 
     switch (type) {
     case REPORT_TYPE_ERROR:
         break;
     case REPORT_TYPE_WARNING:
-        error_printf("warning: ");
+        l->print(opaque, "warning: ");
         break;
     case REPORT_TYPE_INFO:
-        error_printf("info: ");
+        l->print(opaque, "info: ");
         break;
     }
 
-    error_vprintf(fmt, ap);
-    error_printf("\n");
+    l->vprint(opaque, fmt, ap);
+    l->print(opaque, "\n");
 }
 
 /*
-- 
2.43.0

Re: [RFC 0/4] util: qmessage_context followup

[Daniel P. Berrangé]

On Tue, Sep 02, 2025 at 12:30:06PM +0200, Richard Henderson wrote:
> Hi Daniel,
> 
> I'm still not keen on qmessage_context allocating a string.
> 
> If we *did* allocate a string, it should be a GString so that we can
> easily append to that.  The only benefit I see from this is that we
> would collect all of the output and it would reach stderr atomically.
> 
> However, I think it's better to not collect the message and just
> output the pieces.  Something like the following.  The names are
> horrible and I didn't document the patches well.

Thanks for the suggestion, I'll have a look and see about merging
this, or something like it, into my series for a new posting.

> 
> 
> r~
> 
> 
> Richard Henderson (4):
>   util: Introduce LogOutput
>   util: Drop QMESSAGE_CONTEXT_SKIP_MONITOR
>   util/message: Use LogOutput
>   util/error-report: Use LogOutput in vreport
> 
>  include/monitor/monitor.h |  4 +++
>  include/qemu/log-output.h | 14 ++++++++
>  include/qemu/message.h    | 21 +++--------
>  monitor/monitor.c         | 54 +++++++++++++++++++++++------
>  stubs/error-printf.c      | 11 ++++++
>  util/error-report.c       | 73 +++++++++++++++++++--------------------
>  util/log.c                | 23 +++++-------
>  util/message.c            | 45 ++++++++++--------------
>  8 files changed, 139 insertions(+), 106 deletions(-)
>  create mode 100644 include/qemu/log-output.h
> 
> -- 
> 2.43.0
> 

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