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

On 09/27/2012 10:44 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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(a)redhat.com&gt; > --- > 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().

> + > + > +# 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).

> + * 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).

> + /* 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.

> + 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?

> @@ -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?

> + 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: