[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]