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

[Markus Armbruster <armbru@redhat.com>]

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

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

Looking at the bright side (for you): you get to pick the reviewer to
side with!

>> > +    }
>> > +}
>> 
>> 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 :-)

I like local consistency.

Actually, I'd like global consistency, but that's a lost cause in QEMU.

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

qemu_msg_ is exactly as long as qmessage_.  If one is overkill, so is
the other :)

> 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