On Wed, Sep 11, 2013 at 01:49:36PM +0200, Peter Meerwald wrote:
> Hi,
>
> > This patch adds support for logging directly to the journal using its
> > native API.
>
> does this have any advantages over using syslog?
Yes. As already mentioned in the commit message, one of the biggest
advantages over syslog is the advanced filtering ability of log messages.
The only downside in comparison to syslog is that the logs are not stored
as plain text and you need to use a tool like journalctl to view the log
messages.
Some more advantages:
* Lines of error priority (and higher) will be highlighted red.
* Lines of notice/warning priority will be highlighted bold.
* The timestamps are converted into your local time-zone.
* The output is auto-paged with your pager of choice (defaults to less).
More info on this:
http://0pointer.de/blog/projects/journalctl.html
> I suggest to make the relation between 'journal' and systemd more clear in
> configure (so one knows what to install)
In what way? "--disable-systemd-journal"? I thought that this would be
to long and figured that if someone knows what the journal is he/she
knows that it's related to systemd or if someone knows about systemd
he/she knows that the journal is responsible for most (or all) of the
logging on the system.
> the default log target (if deamonized) could be changed from syslog to
> journal if available
>
This is true. But I thought that it would annoy to many people that way.
Distributions can change the default log target anyways. But if everyone
else seems to be fine with this I'll add it.
> what version of systemd is required?
As of v38+ (released 25.08.2011) systemd ships with the journal. (The current stable version
of systemd is 206).
> one nitpick inline
>
> regards, p.
>
> > ---
> > configure.ac | 18 ++++++++++++++++++
> > man/pulse-daemon.conf.5.xml.in | 10 +++++-----
> > man/pulseaudio.1.xml.in | 5 +++--
> > src/Makefile.am | 5 +++++
> > src/daemon/cmdline.c | 4 ++++
> > src/pulsecore/log.c | 39 +++++++++++++++++++++++++++++++++++++++
> > src/pulsecore/log.h | 3 +++
> > 7 files changed, 77 insertions(+), 7 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 616a990..f5456f6 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1145,6 +1145,22 @@ AC_SUBST(HAVE_SYSTEMD)
> > AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$HAVE_SYSTEMD" = x1])
> > AS_IF([test "x$HAVE_SYSTEMD" = "x1"], AC_DEFINE([HAVE_SYSTEMD], 1, [Have SYSTEMD?]))
> >
> > +#### journal support (optional) ####
> > +
> > +AC_ARG_ENABLE([journal],
> > + AS_HELP_STRING([--disable-journal],[Disable optional journal support]))
> > +
> > +AS_IF([test "x$enable_journal" != "xno"],
> > + [PKG_CHECK_MODULES(JOURNAL, [ libsystemd-journal ], HAVE_JOURNAL=1, HAVE_JOURNAL=0)],
> > + HAVE_JOURNAL=0)
> > +
> > +AS_IF([test "x$enable_journal" = "xyes" && test "x$HAVE_JOURNAL" = "x0"],
> > + [AC_MSG_ERROR([*** Needed journal support not found])])
> > +
> > +AC_SUBST(HAVE_JOURNAL)
> > +AM_CONDITIONAL([HAVE_JOURNAL], [test "x$HAVE_JOURNAL" = x1])
> > +AS_IF([test "x$HAVE_JOURNAL" = "x1"], AC_DEFINE([HAVE_JOURNAL], 1, [Have JOURNAL?]))
> > +
> > #### Build and Install man pages ####
> >
> > AC_ARG_ENABLE([manpages],
> > @@ -1389,6 +1405,7 @@ AS_IF([test "x$HAVE_XEN" = "x1"], ENABLE_XEN=yes, ENABLE_XEN=no)
> > AS_IF([test "x$HAVE_DBUS" = "x1"], ENABLE_DBUS=yes, ENABLE_DBUS=no)
> > AS_IF([test "x$HAVE_UDEV" = "x1"], ENABLE_UDEV=yes, ENABLE_UDEV=no)
> > AS_IF([test "x$HAVE_SYSTEMD" = "x1"], ENABLE_SYSTEMD=yes, ENABLE_SYSTEMD=no)
> > +AS_IF([test "x$HAVE_JOURNAL" = "x1"], ENABLE_JOURNAL=yes, ENABLE_JOURNAL=no)
> > AS_IF([test "x$HAVE_BLUEZ" = "x1"], ENABLE_BLUEZ=yes, ENABLE_BLUEZ=no)
> > AS_IF([test "x$HAVE_HAL_COMPAT" = "x1"], ENABLE_HAL_COMPAT=yes, ENABLE_HAL_COMPAT=no)
> > AS_IF([test "x$HAVE_TCPWRAP" = "x1"], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no)
> > @@ -1444,6 +1461,7 @@ echo "
> > Enable udev: ${ENABLE_UDEV}
> > Enable HAL->udev compat: ${ENABLE_HAL_COMPAT}
> > Enable systemd login: ${ENABLE_SYSTEMD}
> > + Enable systemd journal: ${ENABLE_JOURNAL}
> > Enable TCP Wrappers: ${ENABLE_TCPWRAP}
> > Enable libsamplerate: ${ENABLE_LIBSAMPLERATE}
> > Enable IPv6: ${ENABLE_IPV6}
> > diff --git a/man/pulse-daemon.conf.5.xml.in b/man/pulse-daemon.conf.5.xml.in
> > index c40d570..bab2873 100644
> > --- a/man/pulse-daemon.conf.5.xml.in
> > +++ b/man/pulse-daemon.conf.5.xml.in
> > @@ -297,11 +297,11 @@ USA.
> >
> > <option>
> > <p><opt>log-target=</opt> The default log target. Use either
> > - <opt>stderr</opt>, <opt>syslog</opt>, <opt>auto</opt>,
> > - <opt>file:PATH</opt> or <opt>newfile:PATH</opt>. <opt>auto</opt> is
> > - equivalent to <opt>sylog</opt> in case <opt>daemonize</opt> is enabled,
> > - otherwise to <opt>stderr</opt>. If set to <opt>file:PATH</opt>, logging
> > - is directed to the file indicated by PATH. <opt>newfile:PATH</opt> is
> > + <opt>stderr</opt>, <opt>syslog</opt>, <opt>journal</opt> (optional),
> > + <opt>auto</opt>, <opt>file:PATH</opt> or <opt>newfile:PATH</opt>.
> > + <opt>auto</opt> is equivalent to <opt>sylog</opt> in case <opt>daemonize</opt>
>
> sylog -> syslog
Will fix.
> > + is enabled, otherwise to <opt>stderr</opt>. If set to <opt>file:PATH</opt>,
> > + logging is directed to the file indicated by PATH. <opt>newfile:PATH</opt> is
> > otherwise the same as <opt>file:PATH</opt>, but existing files are never
> > overwritten. If the specified file already exists, a suffix is added to
> > the file name to avoid overwriting. Defaults to <opt>auto</opt>. The
> > diff --git a/man/pulseaudio.1.xml.in b/man/pulseaudio.1.xml.in
> > index d7d7458..114cd96 100644
> > --- a/man/pulseaudio.1.xml.in
> > +++ b/man/pulseaudio.1.xml.in
> > @@ -217,12 +217,13 @@ USA.
> > </option>
> >
> > <option>
> > - <p><opt>--log-target</opt><arg>={auto,syslog,stderr,file:PATH,newfile:PATH}</arg></p>
> > + <p><opt>--log-target</opt><arg>={auto,syslog,journal,stderr,file:PATH,newfile:PATH}</arg></p>
> >
> > <optdesc><p>Specify the log target. If set to <arg>auto</arg>
> > (which is the default), then logging is directed to syslog when
> > <opt>--daemonize</opt> is passed, otherwise to
> > - STDERR. If set to <arg>file:PATH</arg>, logging is directed to
> > + STDERR. If set to <arg>journal</arg> logging is directed to the systemd
> > + journal. If set to <arg>file:PATH</arg>, logging is directed to
> > the file indicated by PATH. <arg>newfile:PATH</arg> is otherwise
> > the same as file:PATH, but existing files are never overwritten.
> > If the specified file already exists, a suffix is added to the
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 27477e9..e596235 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -687,6 +687,11 @@ libpulsecommon_ at PA_MAJORMINOR@_la_CFLAGS += $(X11_CFLAGS)
> > libpulsecommon_ at PA_MAJORMINOR@_la_LDFLAGS += $(X11_LIBS)
> > endif
> >
> > +if HAVE_JOURNAL
> > +libpulsecommon_ at PA_MAJORMINOR@_la_CFLAGS += $(JOURNAL_FLAGS)
> > +libpulsecommon_ at PA_MAJORMINOR@_la_LDFLAGS += $(JOURNAL_LIBS)
> > +endif
> > +
> > # proplist-util.h uses these header files, but not the library itself!
> > libpulsecommon_ at PA_MAJORMINOR@_la_CFLAGS += $(GLIB20_CFLAGS) $(GTK30_CFLAGS)
> >
> > diff --git a/src/daemon/cmdline.c b/src/daemon/cmdline.c
> > index 6361a3d..0fa8492 100644
> > --- a/src/daemon/cmdline.c
> > +++ b/src/daemon/cmdline.c
> > @@ -323,7 +323,11 @@ int pa_cmdline_parse(pa_daemon_conf *conf, int argc, char *const argv [], int *d
> >
> > case ARG_LOG_TARGET:
> > if (pa_daemon_conf_set_log_target(conf, optarg) < 0) {
> > +#ifdef HAVE_JOURNAL
> > + pa_log(_("Invalid log target: use either 'syslog', 'journal','stderr' or 'auto' or a valid file name 'file:<path>', 'newfile:<path>'."));
> > +#else
> > pa_log(_("Invalid log target: use either 'syslog', 'stderr' or 'auto' or a valid file name 'file:<path>', 'newfile:<path>'."));
> > +#endif
> > goto fail;
> > }
> > break;
> > diff --git a/src/pulsecore/log.c b/src/pulsecore/log.c
> > index a25a760..bdce8f4 100644
> > --- a/src/pulsecore/log.c
> > +++ b/src/pulsecore/log.c
> > @@ -39,6 +39,10 @@
> > #include <syslog.h>
> > #endif
> >
> > +#ifdef HAVE_JOURNAL
> > +#include <systemd/sd-journal.h>
> > +#endif
> > +
> > #include <pulse/gccmacro.h>
> > #include <pulse/rtclock.h>
> > #include <pulse/utf8.h>
> > @@ -89,6 +93,18 @@ static const int level_to_syslog[] = {
> > };
> > #endif
> >
> > +/* These are actually equivalent to the syslog ones
> > + * but we don't want to depend on syslog.h */
> > +#ifdef HAVE_JOURNAL
> > +static const int level_to_journal[] = {
> > + [PA_LOG_ERROR] = 3,
> > + [PA_LOG_WARN] = 4,
> > + [PA_LOG_NOTICE] = 5,
> > + [PA_LOG_INFO] = 6,
> > + [PA_LOG_DEBUG] = 7
> > +};
> > +#endif
> > +
> > static const char level_to_char[] = {
> > [PA_LOG_ERROR] = 'E',
> > [PA_LOG_WARN] = 'W',
> > @@ -128,6 +144,9 @@ int pa_log_set_target(pa_log_target *t) {
> > switch (t->type) {
> > case PA_LOG_STDERR:
> > case PA_LOG_SYSLOG:
> > +#ifdef HAVE_JOURNAL
> > + case PA_LOG_JOURNAL:
> > +#endif
> > case PA_LOG_NULL:
> > break;
> > case PA_LOG_FILE:
> > @@ -464,6 +483,17 @@ void pa_log_levelv_meta(
> > }
> > #endif
> >
> > +#ifdef HAVE_JOURNAL
> > + case PA_LOG_JOURNAL:
> > + sd_journal_send("MESSAGE=%s", t,
>
> return value?
Hmm thats true. What would you recommend to do if this fails?
Fall back to stderr like we do if writing to a file fails?
> just wondering: syslog and stderr convert utf8 to locale, is journal utf8?
As per man page for sd_journal_send:
It is highly recommended to submit text strings formatted in the UTF-8
character encoding only, and submit binary fields only when formatting
in UTF-8 strings is not sensible.
> > + "PRIORITY=%i", level_to_journal[level],
> > + "CODE_FILE=%s", file,
> > + "CODE_FUNC=%s", func,
> > + "CODE_LINE=%d", line,
> > + NULL);
> > + break;
> > +#endif
> > +
> > case PA_LOG_FILE:
> > case PA_LOG_NEWFILE: {
> > char *local_t;
> > @@ -569,6 +599,10 @@ pa_log_target *pa_log_parse_target(const char *string) {
> > t = pa_log_target_new(PA_LOG_STDERR, NULL);
> > else if (pa_streq(string, "syslog"))
> > t = pa_log_target_new(PA_LOG_SYSLOG, NULL);
> > +#ifdef HAVE_JOURNAL
> > + else if (pa_streq(string, "journal"))
> > + t = pa_log_target_new(PA_LOG_JOURNAL, NULL);
> > +#endif
> > else if (pa_streq(string, "null"))
> > t = pa_log_target_new(PA_LOG_NULL, NULL);
> > else if (pa_startswith(string, "file:"))
> > @@ -593,6 +627,11 @@ char *pa_log_target_to_string(const pa_log_target *t) {
> > case PA_LOG_SYSLOG:
> > string = pa_xstrdup("syslog");
> > break;
> > +#ifdef HAVE_JOURNAL
> > + case PA_LOG_JOURNAL:
> > + string = pa_xstrdup("journal");
> > + break;
> > +#endif
> > case PA_LOG_NULL:
> > string = pa_xstrdup("null");
> > break;
> > diff --git a/src/pulsecore/log.h b/src/pulsecore/log.h
> > index e7dca88..5e9611d 100644
> > --- a/src/pulsecore/log.h
> > +++ b/src/pulsecore/log.h
> > @@ -35,6 +35,9 @@
> > typedef enum pa_log_target_type {
> > PA_LOG_STDERR, /* default */
> > PA_LOG_SYSLOG,
> > +#ifdef HAVE_JOURNAL
> > + PA_LOG_JOURNAL, /* systemd journal */
> > +#endif
> > PA_LOG_NULL, /* to /dev/null */
> > PA_LOG_FILE, /* to a user specified file */
> > PA_LOG_NEWFILE, /* with an automatic suffix to avoid overwriting anything */
> >
>
> --
>
> Peter Meerwald
> +43-664-2444418 (mobile)
