Re: [PATCH v6 22/27] util: introduce common helper for error-report & log code

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

On Wed, Feb 18, 2026 at 03:04:23PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The error-report and log code both have a need to add prefixes
> > to messages they are printing, with the current example being
> > a timestamp.
> >
> > The format and configuration they use should be consistent, so
> > providing a common helper will ensure this is always the case.
> > Initially the helper only emits a timestamp, but future patches
> > will expand this.
> >
> > This takes the liberty of assigning the new file to the same
> > maintainer as the existing error-report.c file, given it will
> > be extracting some functionality from the latter.
> 
> Fair.
> 
> > While vreport() dynamically changes between reporting to the
> > monitor vs stderr, depending on whether HMP is active or not,
> > message prefixes are only ever used in the non-HMP case. Thus
> > the helper API can take a FILE * object and not have to deal
> > with the monitor at all.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  MAINTAINERS            |  2 ++
> >  include/qemu/message.h | 28 ++++++++++++++++++++++++++++
> >  util/meson.build       |  1 +
> >  util/message.c         | 23 +++++++++++++++++++++++
> >  4 files changed, 54 insertions(+)
> >  create mode 100644 include/qemu/message.h
> >  create mode 100644 util/message.c

snip

> > diff --git a/util/message.c b/util/message.c
> > new file mode 100644
> > index 0000000000..99a403f9d0
> > --- /dev/null
> > +++ b/util/message.c
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "qemu/message.h"
> > +#include "monitor/monitor.h"
> 
> Superfluous #include.
> 
> It'll become used in PATCH 26, for qemu_thread_get_name().  Should
> include qemu/thread.h there instead.

opps, yes.

> > +
> > +static int message_format;
> > +
> > +void qmessage_set_format(int flags)
> > +{
> > +    message_format = flags;
> > +}
> > +
> > +void qmessage_context_print(FILE *fp)
> > +{
> > +    if (message_format & QMESSAGE_FORMAT_TIMESTAMP) {
> > +        g_autoptr(GDateTime) dt = g_date_time_new_now_utc();
> > +        g_autofree char *timestr = g_date_time_format_iso8601(dt);
> > +        fputs(timestr, fp);
> > +        fputc(' ', fp);
> 
> The context string is either empty, or it ends with a space, for ease of
> use.  Okay.
> 
> I'd go for
> 
>            fprintf(fp, "%s ", timestr);

Previous reviewer comments preferred fputs/c to avoid
redundant printf string interpolation since qemu_log
could be used in fairly hot code paths  at times.

> 
> > +    }
> > +}
> 
> Alright, everybody's favorite topic: naming.
> 
> message.[ch] aren't about messages, but message *prefixes*.  You call
> them "context" in qmessage_context_print().  I'm fine with "context".

The use of "message" was a somewhat forward looking thing, guessing
at possible other needs related to message ouput.

In particular I think there's scope for the Location handling APIs to
be in this file instead of error-report.c.

So one could imagine  qmessage_loc_push/pop  APis later.

> External symbols are prefixed with qmessage_.  I prefer such prefixes to
> match the filename.

My view is that they do match if you pretend the 'q' is implicit by
this living inside qemu.git ;-P

> Prefix in util/ overwhelmingly start with qemu_.

Naturally my choice was based on what I've previously done for
naming in io/, crypto/ and auth/, etc where all the C APIs have
a leading 'q' to scope them to QEMU, but this is omitted in the
directory/file names :-)

> Somewhat long prefixes feel okay here, as these symbols are used only a
> couple of times.  qemu_message_context_ might be too long, though.
> 
> Could use the classic technique of murdering vowels: to qemu_msg_ctxt_.
> 
> Thoughts?

IMHO, despite the existing usage in util/, "qemu_" is overkill as a
naming prefix.

A plain 'q' prefix is most liable to clash with "Qt" library functions,
but we don't consume that in QEMU so largely not neccessary to worry
about unless perhaps dragged in indirectly via a 3rd party dep

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|