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
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
Rebased on current master.
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.
From 0f55887f603d7492e2dd0a4a2b5dfe4e02a734ae Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Thu, 5 Sep 2013 06:52:15 +0200 Subject: [PATCH 1/2] Add journald support
Makefile.am | 6 ++++++ configure.ac | 6 ++++++ src/conf_macros.m4 | 22 ++++++++++++++++++++++ src/external/systemd.m4 | 13 +++++++++++++ src/util/sss_log.c | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+)
diff --git a/Makefile.am b/Makefile.am index 610f149984d6fd9c98adc95a6dd1fac21c3f87cf..034002324b5c0014db2e79341f71695d6265e190 100644 --- a/Makefile.am +++ b/Makefile.am @@ -279,6 +279,7 @@ AM_CPPFLAGS = \ $(LIBNL_CFLAGS) \ $(OPENLDAP_CFLAGS) \ $(GLIB2_CFLAGS) \
- $(JOURNALD_CFLAGS) \ -DLIBDIR="$(libdir)" \ -DVARDIR="$(localstatedir)" \ -DSHLIBEXT="$(SHLIBEXT)" \
@@ -516,6 +517,10 @@ if HAVE_PTHREAD CLIENT_LIBS += -lpthread endif
+if WITH_JOURNALD +SYSLOG_LIBS = $(JOURNALD_LIBS) +endif
##################### # Utility libraries # ##################### @@ -524,6 +529,7 @@ libsss_debug_la_SOURCES = \ src/util/debug.c \ src/util/sss_log.c libsss_debug_la_LDFLAGS = \
- $(SYSLOG_LIBS) \ -avoid-version
pkglib_LTLIBRARIES += libsss_child.la diff --git a/configure.ac b/configure.ac index c389bec4f2600568e73be4034d29ebcc44bb047f..d28d55f3b0eb35cc4ecc02ae9b1fafbcb9588dcf 100644 --- a/configure.ac +++ b/configure.ac @@ -125,6 +125,7 @@ WITH_SUDO_LIB_PATH WITH_AUTOFS WITH_SSH WITH_CRYPTO +WITH_SYSLOG
m4_include([src/external/pkg.m4]) m4_include([src/external/libpopt.m4]) @@ -245,6 +246,11 @@ if test x$HAVE_SYSTEMD_UNIT != x; then AM_CHECK_SYSTEMD fi
+dnl If journald was selected for logging, configure journald +if test x$syslog = xjournald; then
- AM_CHECK_JOURNALD
+fi
if test x$cryptolib = xnss; then AM_CHECK_NSS fi diff --git a/src/conf_macros.m4 b/src/conf_macros.m4 index b4db0b4a2b34e69734402f2f4a4c2ccadbf94fb8..7f99cbb870b258ad35a51dbe0e6e044c57fcd11a 100644 --- a/src/conf_macros.m4 +++ b/src/conf_macros.m4 @@ -152,6 +152,28 @@ AC_DEFUN([WITH_INITSCRIPT], AC_MSG_NOTICE([Will use init script type: $initscript]) ])
+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"] )
- if test x"$with_syslog" = xsyslog || \
test x"$with_syslog" = xjournald; thensyslog=$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
- ])
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.
+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 automaticallydnl while setting CFLAGS and LIBS manually twice doesn't hurt.AC_SUBST([JOURNALD_CFLAGS])AC_SUBST([JOURNALD_LIBS])+]) diff --git a/src/util/sss_log.c b/src/util/sss_log.c index 45e883109a0a45a146b62dcedf43113147e78c28..6b78c9d4baa6ee9bc15d461ad8136c7bcfca579a 100644 --- a/src/util/sss_log.c +++ b/src/util/sss_log.c @@ -23,7 +23,12 @@ */
#include "util/util.h"
+#ifdef WITH_JOURNALD +#include <systemd/sd-journal.h> +#else /* WITH_JOURNALD */ #include <syslog.h> +#endif /* WITH_JOURNALD */
static int sss_to_syslog(int priority) { @@ -52,6 +57,34 @@ static int sss_to_syslog(int priority) } }
+#ifdef WITH_JOURNALD
+void sss_log(int priority, const char *format, ...) +{
- va_list ap;
- int syslog_priority;
- int ret;
- char *message;
- va_start(ap, format);
- ret = vasprintf(&message, format, ap);
- va_end(ap);
- if (ret == -1) {
/* ENOMEM */return;- }
- syslog_priority = sss_to_syslog(priority);
- sd_journal_send("MESSAGE=%s", message,
"PRIORITY=%i", syslog_priority,"SYSLOG_FACILITY=%i", LOG_FAC(LOG_DAEMON),"SYSLOG_IDENTIFIER=%s", debug_prg_name,NULL);+}
+#else /* WITH_JOURNALD */
void sss_log(int priority, const char *format, ...) { va_list ap; @@ -67,3 +100,5 @@ void sss_log(int priority, const char *format, ...)
closelog();}
+#endif /* WITH_JOURNALD */
1.8.3.1
From 32d4415a36da7eaa165dce5073bad4d252814b6c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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.
Would it be better to have unsetenv and setenv in the one place? I am not sure about this.
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 = ""; ^ ~~
LS
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; thensyslog=$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
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 automaticallydnl 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@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.
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.
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; thensyslog=$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 automaticallydnl 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@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
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; thensyslog=$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 automaticallydnl 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@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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik