On Dec 21, 2013, at 1:25 PM, Lukas Slebodnik
<lslebodn(a)redhat.com> wrote:
> On (20/12/13 17:05), Stephen Gallagher wrote:
> All of these patches require Nikolai's "DEBUG Macro Refactoring v3"
> patches to be applied first.
>
>
>
>
> Patch 0001: Allow debug_fn to process __FILE__ and __LINE__
>
> In preparation for enabling journald support for the DEBUG logs, we
> will need to be able to pass in certain additional arguments that will
> be required, specifically the code file and line number.
>
> We will be able to optionally enable this in the file-based logs as well
> if we so choose, but for right now we will avoid breaking the log
> format on disk.
>
>
> Patch 0002: Enable sending structured debug logs to journald
>
> We are now able to send structured debug logs to journald, tagged with
> the code file, line number and domain that the log pertains to. To
> enable this functionality, SSSD must be configured at build-time with
> --with-syslog=journald and must be launched without -f/--debug-to-files
>
> This behavior is nearly identical to how SSSD will function today on a
> systemd-based system if --debug-to-files is disabled, since it will
> redirect stdout and stderr into journald. This patch merely enhances
> the situation to send structured logs instead of simple string messages.
>
>
> Patch 0003: BUILD: Build with journald support by default on Fedora
>
> The journal provided by systemd gives us structured logging
> capabilities that we should be taking advantage of.
>
> Note: this patch explicitly does not change the systemd unit file for
> SSSD. Right now, an administrator will need to manually remove the
> '-f' from ExecStart in the unit file to send debug logs to journald. I
> suspect we'll want to discuss this before we make it the default. This
> patch DOES change the default for sss_log messages to use
> sd_journal_send() instead of straight log() for those messages that we
> traditionally sent to the syslog (such as login events). This is code
> that has been in place for some time now, but has not been the default
> because we hadn't build with --with-syslog=journald.
> From e52fdecc33b0e8b2df8e983ee145ab421a6fffc2 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh(a)redhat.com>
> Date: Fri, 20 Dec 2013 15:17:19 -0500
> Subject: [PATCH 1/3] DEBUG: Allow debug_fn to process __FILE__ and __LINE__
>
> In preparation for enabling journald support for the DEBUG logs, we
> will need to be able to pass in certain additional arguments that will
> be required, specifically the code file and line number.
>
> We will be able to optionally enable this in the file-based logs as well
> if we so choose, but for right now we will avoid breaking the log
> format on disk.
> ---
> src/tools/selinux.c | 2 +-
> src/util/debug.c | 8 ++++++--
> src/util/util.h | 14 ++++++++++----
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/src/tools/selinux.c b/src/tools/selinux.c
> index
b3565de3de1f35218afbb7e3bf3e3362ae8122ec..5905461ca1d09673409cc05d519b6c00a83ca3d9 100644
> --- a/src/tools/selinux.c
> +++ b/src/tools/selinux.c
> @@ -121,7 +121,7 @@ static void sss_semanage_error_callback(void *varg,
> return;
> }
>
> - debug_fn("libsemanage", level, "%s\n", message);
> + debug_fn(__FILE__, __LINE__, "libsemanage", level, "%s\n",
message);
> free(message);
> }
>
> diff --git a/src/util/debug.c b/src/util/debug.c
> index
0aff2da9e5eaeb468c9502d2686f78da9cc31056..b3304a859e91b2509746c5ffbd101cf73b711fce 100644
> --- a/src/util/debug.c
> +++ b/src/util/debug.c
> @@ -128,7 +128,11 @@ static void debug_printf(const char *format, ...)
> va_end(ap);
> }
>
> -void debug_fn(const char *function, int newlevel, const char *format, ...)
> +void debug_fn(const char *file,
> + long line,
> + const char *function,
> + int newlevel,
> + const char *format, ...)
> {
> va_list ap;
> if (debug_timestamps) {
> @@ -190,7 +194,7 @@ void ldb_debug_messages(void *context, enum ldb_debug_level
level,
> return;
> }
>
> - debug_fn("ldb", loglevel, "%s\n", message);
> + debug_fn(__FILE__, __LINE__, "ldb", loglevel, "%s\n",
message);
>
> free(message);
> }
> diff --git a/src/util/util.h b/src/util/util.h
> index
7d9a5264d461e3bcf511187320050936f44da70f..b3bb740f7780ce71f6726099d51ccac0065d3c9d 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -68,8 +68,11 @@ extern int debug_timestamps;
> extern int debug_microseconds;
> extern int debug_to_file;
> extern const char *debug_log_file;
> -void debug_fn(const char *function, int newlevel, const char *format, ...)
> - SSS_ATTRIBUTE_PRINTF(3, 4);
> +void debug_fn(const char *file,
> + long line,
> + const char *function,
> + int newlevel,
> + const char *format, ...);
> int debug_convert_old_level(int old_level);
> errno_t set_debug_file_from_fd(const int fd);
NACK
Why do you removed SSS_ATTRIBUTE_PRINTF ?
We had some crashes in sssd because of wrong debug message.
For example: e9ea740625881291893b006ffad906d4861c668d
"Fix segfault in DYNDNS"
I think that was probably me hitting ctrl-d one too many times and not noticing that I
removed it. It wasn't intentional. I'll send a new version of the patch with that
added back in when I am back from vacation in the new year (or it can be fixed at push, if
someone else could test and verify it doesn't break anything.)