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(a)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