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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list