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