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:

> 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

No uses, yet.  Commit message could mention it's put to use in the next
few commits.

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29f88d48f3..96a4ce438e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3261,9 +3261,11 @@ M: Markus Armbruster <armbru@redhat.com>
>  S: Supported
>  F: include/qapi/error.h
>  F: include/qemu/error-report.h
> +F: include/qemu/message.h
>  F: qapi/error.json
>  F: util/error.c
>  F: util/error-report.c
> +F: util/message.c
>  F: scripts/coccinelle/err-bad-newline.cocci
>  F: scripts/coccinelle/error-use-after-free.cocci
>  F: scripts/coccinelle/error_propagate_null.cocci
> diff --git a/include/qemu/message.h b/include/qemu/message.h
> new file mode 100644
> index 0000000000..0a06421f77
> --- /dev/null
> +++ b/include/qemu/message.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef QEMU_MESSAGE_H
> +#define QEMU_MESSAGE_H
> +
> +enum QMessageFormatFlags {
> +    QMESSAGE_FORMAT_TIMESTAMP = (1 << 0),
> +};
> +
> +/**
> + * qmessage_set_format:
> + * @flags: the message information to emit
> + *
> + * Select which pieces of information to
> + * emit for messages
> + */
> +void qmessage_set_format(int flags);
> +
> +/**
> + * qmessage_context_print:
> + * @fp: file to emit the prefix on
> + *
> + * Emit a message prefix with the information selected by
> + * an earlier call to qmessage_set_format.
> + */
> +void qmessage_context_print(FILE *fp);
> +
> +#endif /* QEMU_MESSAGE_H */
> diff --git a/util/meson.build b/util/meson.build
> index 7c9445615d..3035bd4bee 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -38,6 +38,7 @@ util_ss.add(files('host-utils.c'))
>  util_ss.add(files('bitmap.c', 'bitops.c'))
>  util_ss.add(files('fifo8.c'))
>  util_ss.add(files('cacheflush.c'))
> +util_ss.add(files('message.c'))
>  util_ss.add(files('error.c', 'error-report.c'))
>  util_ss.add(files('qemu-print.c'))
>  util_ss.add(files('id.c'))
> 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.

> +
> +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);

> +    }
> +}

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

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

Prefix in util/ overwhelmingly start with 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?