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.