[SSSD] [PATCH] Add journald support

Jakub Hrozek jhrozek at redhat.com
Wed Sep 18 17:00:45 UTC 2013


On Wed, Sep 18, 2013 at 03:24:28PM +0200, Lukas Slebodnik wrote:
> On (17/09/13 20:27), Jakub Hrozek wrote:
> >On Tue, Sep 17, 2013 at 04:12:25PM +0200, Lukas Slebodnik wrote:
> >> On (16/09/13 17:24), Jakub Hrozek wrote:
> >> >On Tue, Sep 10, 2013 at 07:35:05PM +0200, Jakub Hrozek wrote:
> >> >> Hi,
> >> >> 
> >> >> attached are two small patches I wrote when I was checking whether
> >> >> Fedora's move to journald affects us or not. Patch #1 adds a new
> >> >> configure option, that, if enabled, routes logging via journald's API.
> >> >> No change in logging format is present in this patch. You need to have
> >> >> systemd-devel installed -- if the patches are accepted I'll amend the
> >> >> specfile as well, but I didn't want to waste time now.
> >> >> 
> >> >> Patch #2 adds acts as kind of a demonstration of journald's extended
> >> >> capabilities. The patch adds a new field that contains SSSD domain name
> >> >> and makes it easy to distinguish log messages coming from different
> >> >> domains with:
> >> >>     # journalctl SSSD_DOMAIN=foo.example.com
> >> 
> >> The patches work fine from functional point of view.
> >> Function sss_log is used very rarely, therefore the patches do not have big
> >> effect. But on the other hand, it is nice as a proof of concept.
> >> 
> >> >
> >> >Rebased on current master.
> >> 
> >
> >Hi,
> >
> >thank you for the review.
> >
> >> >+AC_DEFUN([WITH_SYSLOG],
> >> >+  [ AC_ARG_WITH([syslog],
> >> >+                [AC_HELP_STRING([--with-syslog=SYSLOG_TYPE],
> >> >+                                [Type of your system logger (syslog|journald). [syslog]]
> >> >+                               )
> >> >+                ]
> >> >+               )
> >> >+  default_syslog=syslog
> >> >+  if test x"$with_syslog" = x; then
> >> >+    with_syslog=$default_syslog
> >> >+  fi
> >> You can use 4th optional argument of AC_ARG_WITH instead of these 4 lines
> >>    AC_ARG_WITH (package, help-string, [action-if-given], [action-if-not-given])
> >> 
> >> Somethink like:
> >> AC_ARG_WITH([syslog],
> >>             [AC_HELP_STRING( <snip />  )],
> >>             [],     dnl default action-if-given
> >>             [with_syslog="syslog"]
> >> )
> >> 
> >> 
> >
> >Done. I think I copied the block from other configure snippets that
> >don't use this option either.
> >
> >> >+
> >> >+  if test x"$with_syslog" = xsyslog || \
> >> >+     test x"$with_syslog" = xjournald; then
> >> >+        syslog=$with_syslog
> >> >+  else
> >> >+      AC_MSG_ERROR([Uknown syslog type, supported types are syslog and journald])
> >> >+  fi
> >> >+
> >> >+  AM_CONDITIONAL([WITH_JOURNALD], [test x"$syslog" = xjournald])
> >> I think this line fits better to macro AM_CHECK_JOURNALD
> >
> >Yes, but AM_CONDITIONAL documentation says that:
> >
> >    Note that you must arrange for every AM_CONDITIONAL to be invoked every
> >    time configure is run. If AM_CONDITIONAL is run conditionally (e.g., in
> >    a shell if statement), then the result will confuse automake
> >
> Yes, I forgot about this.
> 
> >And AM_CHECK_JOURNALD is called in an if condition, so we would confuse
> >automake :-)
> >
> >I can move it to configure.ac after the call to AM_CHECK_JOURNALD if you
> >prefer.
> >
> >> 
> >> >+  ])
> >> >+
> >> > AC_DEFUN([WITH_ENVIRONMENT_FILE],
> >> >   [ AC_ARG_WITH([environment_file],
> >> >                 [AC_HELP_STRING([--with-environment-file=PATH], [Path to environment file [/etc/sysconfig/sssd]])
> >> >diff --git a/src/external/systemd.m4 b/src/external/systemd.m4
> >> >index 202915a560e54ba92912ad8f289ae33e1d1a001f..8e0a5b6a79a97f5e77a2b5fb0204dcc815dbe85e 100644
> >> >--- a/src/external/systemd.m4
> >> >+++ b/src/external/systemd.m4
> >> >@@ -6,7 +6,20 @@ AC_DEFUN([AM_CHECK_SYSTEMD],
> >> >                      [AC_MSG_ERROR([Could not detect systemd presence])]
> >> >                     )
> >> > ])
> >> >+
> >> > AM_COND_IF([HAVE_SYSTEMD],
> >> >            [PKG_CHECK_MODULES([SYSTEMD_LOGIN], [libsystemd-login],
> >> >             [AC_DEFINE_UNQUOTED(HAVE_SYSTEMD_LOGIN, 1, [Build with libsystemdlogin support])],
> >> >             [AC_DEFINE_UNQUOTED(HAVE_SYSTEMD_LOGIN, 0, [Build without libsystemd-login support])])])
> >> Here is a conflict with Sumit's patch
> >>   "Do not set HAVE_SYSTEMD_LOGIN if libsystemd-login is not available"
> >> The fix is simple.
> >> 
> >
> >Done.
> >
> >> >+
> >> >+dnl A macro to check presence of journald on the system
> >> >+AC_DEFUN([AM_CHECK_JOURNALD],
> >> >+[
> >> >+       PKG_CHECK_MODULES(JOURNALD,
> >> >+                         libsystemd-journal,
> >> >+                         [AC_DEFINE_UNQUOTED([WITH_JOURNALD], 1, [journald is available])])
> >> >+       dnl Some older versions of pkg-config might not set these automatically
> >> >+       dnl while setting CFLAGS and LIBS manually twice doesn't hurt.
> >> >+       AC_SUBST([JOURNALD_CFLAGS])
> >> >+       AC_SUBST([JOURNALD_LIBS])
> >> >+])
> >
> >[snip]
> >
> >> >From 32d4415a36da7eaa165dce5073bad4d252814b6c Mon Sep 17 00:00:00 2001
> >> >From: Jakub Hrozek <jhrozek at redhat.com>
> >> >Date: Tue, 10 Sep 2013 19:16:48 +0200
> >> >Subject: [PATCH 2/2] BE: Log domain name to journald if available
> >> >
> >> >If the SSSD is compiled with journald support, then all sss_log()
> >> >statements will include a new field called "SSSD_DOMAIN" that includes
> >> >the domain name. Filtering only messages from the single domain is then
> >> >as easy as:
> >> > # journalctl SSSD_DOMAIN=foo.example.com
> >> >---
> >> > src/providers/data_provider_be.c | 2 ++
> >> > src/util/server.c                | 2 ++
> >> > src/util/sss_log.c               | 7 +++++++
> >> > src/util/util.h                  | 2 ++
> >> > 4 files changed, 13 insertions(+)
> >> >
> >> >diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> >> >index 912b4191c0f6984babf96bb8073db6c01b48afbf..ccd51b45fd9aee25b052f6b7bf7f869dc234c138 100644
> >> >--- a/src/providers/data_provider_be.c
> >> >+++ b/src/providers/data_provider_be.c
> >> >@@ -2891,6 +2891,8 @@ int main(int argc, const char *argv[])
> >> >         return 2;
> >> >     }
> >> > 
> >> >+    setenv(SSS_DOM_ENV, be_domain, 1);
> >> >+
> >> >     ret = die_if_parent_died();
> >> >     if (ret != EOK) {
> >> >         /* This is not fatal, don't return */
> >> >diff --git a/src/util/server.c b/src/util/server.c
> >> >index a33207b3da8a713d90dbd840475ecdb75f1fff0c..7a05e1b1f19e9f6129277c23dec71b97cfd1b142 100644
> >> >--- a/src/util/server.c
> >> >+++ b/src/util/server.c
> >> >@@ -423,6 +423,8 @@ int server_setup(const char *name, int flags,
> >> > 
> >> >     setenv("_SSS_LOOPS", "NO", 0);
> >> > 
> >> >+    unsetenv(SSS_DOM_ENV);
> >> Could you write comment why function unsetenv is called.
> >
> >Added.
> >
> >> 
> >> Would it be better to have unsetenv and setenv in the one place?
> >> I am not sure about this.
> >
> >I would prefer it for readability, too, but server_setup() is called for
> >any SSSD subprocess, not just backends. So I wanted to make sure that
> >when the subprocess starts, the variable is not set and the backend
> >would override it itself.
> OK
> 
> >
> >> 
> >> >+
> >> >     setup_signals();
> >> > 
> >> >     /* we want default permissions on created files to be very strict,
> >> >diff --git a/src/util/sss_log.c b/src/util/sss_log.c
> >> >index 6b78c9d4baa6ee9bc15d461ad8136c7bcfca579a..a865814f8346548af8ba0518e47065cd196dd995 100644
> >> >--- a/src/util/sss_log.c
> >> >+++ b/src/util/sss_log.c
> >> >@@ -65,6 +65,7 @@ void sss_log(int priority, const char *format, ...)
> >> >     int syslog_priority;
> >> >     int ret;
> >> >     char *message;
> >> >+    char *domain;
> >> > 
> >> >     va_start(ap, format);
> >> >     ret = vasprintf(&message, format, ap);
> >> >@@ -75,8 +76,14 @@ void sss_log(int priority, const char *format, ...)
> >> >         return;
> >> >     }
> >> > 
> >> >+    domain = getenv(SSS_DOM_ENV);
> >> >+    if (domain == NULL) {
> >> >+        domain = "";
> >> New warning is introduced by this line.
> >> 
> >> src/util/sss_log.c:81:16: error: assigning to 'char *' from 'const char [1]' discards qualifiers
> >>       [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> >>         domain = "";
> >>                ^ ~~
> >> 
> >
> >Grrr, my CFLAGS didn't tell me anything..
> >
> >I converted the "domain" variable to "const char *".
> >
> >Thank you for the review. New patches are attached.
> 
> Works fine, default value is syslog.
> 
> ACK
> 
> LS

Pushed to master.



More information about the sssd-devel mailing list