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
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 <jhro...@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.
>From b2370c2d44ed2d44ed9fc0b81ffc5ef5ea8e2e1c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@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 | 20 ++++++++++++++++++++
src/external/systemd.m4 | 13 +++++++++++++
src/util/sss_log.c | 35 +++++++++++++++++++++++++++++++++++
5 files changed, 80 insertions(+)
diff --git a/Makefile.am b/Makefile.am
index
5aba760375de59c4203300376e2d6fcd8e90c53f..67f5652f7d8373da21773ecc488960631f3a86bc
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)\" \
@@ -517,6 +518,10 @@ if HAVE_PTHREAD
CLIENT_LIBS += -lpthread
endif
+if WITH_JOURNALD
+SYSLOG_LIBS = $(JOURNALD_LIBS)
+endif
+
#####################
# Utility libraries #
#####################
@@ -525,6 +530,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..1aecaea4d60140f9f99d6f6038a4f77205d2e1bd
100644
--- a/src/conf_macros.m4
+++ b/src/conf_macros.m4
@@ -152,6 +152,26 @@ 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]]
+ )
+ ],
+ [],
+ [with_syslog="syslog"]
+ )
+
+ 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])
+ ])
+
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
9afb65def6abb9bfd5b58402c12cf160612cff3b..dbced0d66aa19e064f998648675a5a9c080eaab8
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_MSG_NOTICE([Build without libsystemd-login support])])])
+
+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])
+])
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 598df7f3224f3b997bfc47c9d69300f32dd432ec Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@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 | 5 +++++
src/util/sss_log.c | 7 +++++++
src/util/util.h | 2 ++
4 files changed, 16 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..3dcfccaf6868da9a823621eb8e2b1dfaccb9a4f1
100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -423,6 +423,11 @@ int server_setup(const char *name, int flags,
setenv("_SSS_LOOPS", "NO", 0);
+ /* To make sure the domain cannot be set from the environment, unset the
+ * variable explicitly when setting up any server. Backends later set the
+ * value after reading domain from the configuration */
+ unsetenv(SSS_DOM_ENV);
+
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..5be9e7f2bc1486d83393d2d105c1a73322d7f6f1
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;
+ const 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 = "";
+ }
+
syslog_priority = sss_to_syslog(priority);
sd_journal_send("MESSAGE=%s", message,
+ "SSSD_DOMAIN=%s", domain,
"PRIORITY=%i", syslog_priority,
"SYSLOG_FACILITY=%i", LOG_FAC(LOG_DAEMON),
"SYSLOG_IDENTIFIER=%s", debug_prg_name,
diff --git a/src/util/util.h b/src/util/util.h
index
18ec4176bbe607076fba47e4124fa400e06f9f1f..20d230c125bed0f6c84735a1bfdaebbcad40dbfb
100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -72,6 +72,8 @@ int debug_get_level(int old_level);
int debug_convert_old_level(int old_level);
errno_t set_debug_file_from_fd(const int fd);
+#define SSS_DOM_ENV "_SSS_DOM"
+
#define SSSDBG_FATAL_FAILURE 0x0010 /* level 0 */
#define SSSDBG_CRIT_FAILURE 0x0020 /* level 1 */
#define SSSDBG_OP_FAILURE 0x0040 /* level 2 */
--
1.8.3.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
