Daniel P. Berrangé <[email protected]> writes: > On Wed, Feb 18, 2026 at 03:04:23PM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <[email protected]> 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 <[email protected]> >> > Signed-off-by: Daniel P. Berrangé <[email protected]> >> > --- >> > 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
