Re: [libvirt] [PATCH 09/12] Add a JSON properties parameter to virLog{, V}Message
Thanks for the review.
----- Original Message -----
On Thu, Sep 20, 2012 at 08:24:08PM +0200, Miloslav Trmač wrote:
> ... and update all users. No change in functionality, the parameter
> will be used in later patches.
>
> diff --git a/src/util/logging.h b/src/util/logging.h
> index a66f5dc..14b7b7c 100644
> --- a/src/util/logging.h
> +++ b/src/util/logging.h
> @@ -141,13 +142,13 @@ extern int virLogParseFilters(const char
> *filters);
> extern int virLogParseOutputs(const char *output);
> extern void virLogMessage(const char *category, int priority,
> const char *funcname, long long linenr,
> - unsigned int flags,
> - const char *fmt, ...)
> ATTRIBUTE_FMT_PRINTF(6, 7);
> + virJSONObjectPtr properties, unsigned
> int flags,
> + const char *fmt, ...)
Definite NACK to this change, since it is exposing the impl details
of the Lumberjack log output function to all users of the libvirt
logging API, not to mention the general unpleasant usability aspects
of having to build up JSON objects simply to pass a few extra metadata
fields.
I didn't think adding an abstraction for a single user was worth it, but you're
right, the result did end up rather uglier than I expected.
I'm all for allowing more metadata properties to be passed into
the
logging functions, but we need a simpler API along the lines of the
systemd journal sd_journal_send() style which allows for a set of
key=value pairs to be passed in. I'd not try to shoe-horn it into
the existing virLogMessage() APIs. In the same way that there is
sd_journal_print() for simple string messages, vis sd_journal_send()
for arbitrary key=value pair log messages, I'd create a new API
for this called virLogMetadata and virLogVMetadata. eg to allow
sending a message with an error no
virLogMetadata(__FILE__, VIR_LOG_WARN,
__FUNC__, __LINE__,
0,
"MESSAGE=Unable to open file %s: %s",
"/some/file",
strerror(errno),
"ERRNO=%d", errno,
NULL);
I'm afraid this is not possible to implement portably and reliably without
reimplementing 50% of sprintf(), especially in the presence of translations and the
associated %5$s formats.[1][2]
One problem with the above is that it is possible to send a log event without the MESSAGE
field, which would leave non-structured formats with nothing to log. I can see three
options:
1) as proposed above, silently failing if the MESSAGE field is missing (or if there is a
typo in the field name); with type fields added to represent JSON integers as integers.
2) MESSAGE is mandatory, everything pre-formatted
virAsprintf(&msg, "Unable to open file %s: %s", "/some/file",
strerror(errno));
virLogMetadata(... __LINE__, msg,
"ERRNO", VIR_LOG_INT, errno,
NULL)
3) MESSAGE is printf-like, everything else in an array.
struct virLogMetadata meta[2] = { { "ERRNO", VIR_LOG_INT, .i = errno }, {
NULL, } };
virLogMetadata(... __LINE__, meta,
"Unable to open file %s: %s", "/some/file",
strerror(errno));
or equivalently:
virLogMetadata(... __LINE__,
&(struct virLogMetadata []) {
{ "ERRNO", VIR_LOG_INT, .i = errno },
{ NULL, }
},
"Unable to open file %s: %s", "/some/file",
strerror(errno));
which could be hidden by macros:
virLogMetadata(VIR_LOG_INT("ERRNO", errno"),
VIR_LOG_END,
"Unable to open file %s: %s", "/some/file",
strerror(errno));
Do you have any preference? I'm leaning towards the first variant of 3) for now, we
can add fancy macros later when/if more callers of virLogMetadata are added.
Thanks again,
Mirek
[1] The unportable way to do this is to use parse_printf_format() from glibc.
[2] The logging API could implement only a subset of the printf formats, but it would be
rather difficult to notice if a newly added caller used an unsupported format a few years
later, especially when translators can use different formats than the original string.