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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list