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

Re: [libvirt] [PATCH 7/9] Add systemd journal support



On Thu, Sep 27, 2012 at 02:51:35PM -0600, Eric Blake wrote:
> On 09/27/2012 10:44 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange redhat com>
> > 
> > Add support for logging to the systemd journal, using its
> > simple client library. The benefit over syslog is that it
> > accepts structured log data, so the journald can store
> > individual items like code file/line/func separately from
> > the string message. Tools which require structured log
> > data can then query the journal to extract exactly what
> > they desire without resorting to string parsing
> > 
> > While systemd provides a simple client library for logging,
> > it is more convenient for libvirt to directly write its
> > own client code. This lets us build up the iovec's on
> > the stack, avoiding the need to alloc memory when writing
> > log messages.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange redhat com>
> > ---
> >  cfg.mk             |   2 +-
> >  src/util/logging.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/logging.h |   1 +
> >  3 files changed, 179 insertions(+), 1 deletion(-)
> 
> I'll need a v2 on this one.
> 
> Fails to compile:
> 
> util/logging.c: In function 'virLogOutputToJournald':
> util/logging.c:1124:84: error: ordered comparison of pointer with
> integer zero [-Werror=extra]
> util/logging.c:1124:18: error: ignoring return value of 'virStrcpy',
> declared with attribute warn_unused_result [-Werror=unused-result]
> cc1: all warnings being treated as errors
> 
> > 
> > diff --git a/cfg.mk b/cfg.mk
> > index bbfd4a2..4482d70 100644
> > --- a/cfg.mk
> > +++ b/cfg.mk
> > @@ -771,7 +771,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
> >    ^(bootstrap.conf$$|src/util/util\.c$$|examples/domain-events/events-c/event-test\.c$$)
> >  
> >  exclude_file_name_regexp--sc_prohibit_close = \
> > -  (\.p[yl]$$|^docs/|^(src/util/virfile\.c|src/libvirt\.c)$$)
> > +  (\.p[yl]$$|^docs/|^(src/util/virfile\.c|src/libvirt\.c|src/util/logging\.c)$$)
> 
> That's a bit of a heavy hammer, just to avoid a recursively-logged
> close().  I'd rather omit this and use VIR_LOG_CLOSE().

Ahhh, I didn't notice VIR_LOG_CLOSE. I did indeed hit a
recursively logging close :-)

> > +
> > +
> > +# define IOVEC_SET_STRING(iov, str)          \
> > +    do {                                     \
> > +        struct iovec *_i = &(iov);           \
> > +        _i->iov_base = (char*)str;           \
> > +        _i->iov_len = strlen(str);           \
> > +    } while (0)
> > +
> > +# define IOVEC_SET_INT(iov, buf, fmt, val)      \
> > +    do {                                        \
> > +        struct iovec *_i = &(iov);              \
> > +        snprintf(buf, sizeof(buf), fmt, val);   \
> 
> Technically, snprintf is not async-signal safe, which kind of goes
> against the idea of using it in our logging functions.  I'd much rather
> see us do a hand-rolled conversion of int to %d or %zu (which appear to
> be the only formats you ever pass here).

Ok, should be easy enough, although I notice that the
code we use for generating the timestamp string also
uses snprintf(), so we should fix that sometime too.

> > +         * encode the string length, since we can't
> > +         * rely on the newline for the field separator
> > +         */
> > +        IOVEC_SET_STRING(iov[niov++], "MESSAGE\n");
> > +        nstr = htole64(strlen(rawstr));
> 
> htole64() is a glibc extension which does not exist on mingw, and has
> not (yet) been ported to gnulib.  Then again, this code is inside #ifdef
> HAVE_SYSLOG_H, so you can probably get away with it (although I'm not
> sure whether all platforms that provide <syslog.h> happen to also
> provide htole64(), I at least know that it will exclude mingw).

I think I'll #ifdef __linux__ the whole block just to be sure,
since systemd is only ever going to be Linux based

> > +    /* Message was too large, so dump to temporary file
> > +     * and pass an FD to the journal
> > +     */
> > +
> > +    if ((buffd = mkostemp(path, O_CLOEXEC|O_RDWR)) < 0)
> 
> Is mkostemp async-signal safe?  But if it isn't, I don't know how else
> to generate a safe file name.  Maybe we create ourselves a safe
> temporary directory at process start where we don't care about the async
> safety issues, and then in this function, we track a static counter that
> we increment each time we create a new file within that directory.

Yep, we never need multiple concurrent temp files, so I can just
create one when we initialize logging, and hold open its file
descriptor.

> 
> > +        return;
> > +
> > +    if (unlink(path) < 0)
> > +        goto cleanup;
> > +
> > +    if (writev(buffd, iov, niov) < 0)
> 
> Again, mingw and gnulib lacks writev(), but this function isn't compiled
> on mingw, so we're safe for now.
> 
> > +        goto cleanup;
> > +
> > +    mh.msg_iov = NULL;
> > +    mh.msg_iovlen = 0;
> > +
> > +    memset(&control, 0, sizeof(control));
> > +    mh.msg_control = &control;
> > +    mh.msg_controllen = sizeof(control);
> > +
> > +    cmsg = CMSG_FIRSTHDR(&mh);
> > +    cmsg->cmsg_level = SOL_SOCKET;
> > +    cmsg->cmsg_type = SCM_RIGHTS;
> > +    cmsg->cmsg_len = CMSG_LEN(sizeof(int));
> > +    memcpy(CMSG_DATA(cmsg), &buffd, sizeof(int));
> > +
> > +    mh.msg_controllen = cmsg->cmsg_len;
> > +
> > +    sendmsg(journalfd, &mh, MSG_NOSIGNAL);
> > +
> > +cleanup:
> > +    close(buffd);
> 
> VIR_LOG_CLOSE() works just fine here (but not VIR_FORCE_CLOSE, as that
> is an accident waiting to trigger recursive logging)
> 
> > +}
> > +
> > +
> > +static void virLogCloseJournald(void *data ATTRIBUTE_UNUSED)
> > +{
> > +    close(journalfd);
> > +}
> > +
> > +
> > +static int virLogAddOutputToJournald(int priority)
> > +{
> > +    if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0)
> > +        return -1;
> > +    if (virSetInherit(journalfd, false) < 0) {
> 
> Why not use SOCK_DGRAM | SOCK_CLOEXEC in the socket() call?

It wasn't clear to me whether GNULIB provides compat for
SOCK_CLOEXEC or not. Even though we're only Linux, older
glibc won't provide that

> > @@ -1114,6 +1285,12 @@ virLogParseOutputs(const char *outputs)
> >                  count++;
> >              VIR_FREE(name);
> >              VIR_FREE(abspath);
> > +        } else if (STREQLEN(cur, "journald", 8)) {
> > +            cur += 8;
> > +#if HAVE_SYSLOG_H
> 
> Do we really want to silently parse and ignore 'journald' on systems
> that lack syslog.h?

I just followed the same pattern that we used with the
syslog parsing.

> 
> > +            if (virLogAddOutputToJournald(prio) == 0)
> > +                count++;
> > +#endif /* HAVE_SYSLOG_H */
> >          } else {
> >              goto cleanup;
> >          }
> > diff --git a/src/util/logging.h b/src/util/logging.h
> > index e5918db..d4aa62b 100644
> > --- a/src/util/logging.h
> > +++ b/src/util/logging.h
> > @@ -80,6 +80,7 @@ typedef enum {
> >      VIR_LOG_TO_STDERR = 1,
> >      VIR_LOG_TO_SYSLOG,
> >      VIR_LOG_TO_FILE,
> > +    VIR_LOG_TO_JOURNALD,
> >  } virLogDestination;
> >  
> >  typedef enum {
> > 
> 
> Here's a minimum of what I propose squashing in:

I'll re-post this particular patch for more review.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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