[PATCH 0/2] systemd enhancements
The two patches that follow improve syslog-ng's support for systemd-enabled platforms. The first patch simply teaches the system() source to prefer /run/systemd/journal/syslog over /dev/log if running under systemd. That alone makes configs that use system() work under both sysvinit and systemd without any change at all to the config. However, for configs that use /dev/log hard-coded, the upgrade path isn't that smooth. In that case, we still check whether running under systemd, and if so, but we couldn't accept any of the offered fds, we check again with /run/systemd/journal/syslog as the filename, and replace the hard-coded one if the other filename worked. The second patch implements the above - I'm not entirely sold on the idea, and haven't explored the possible side-effects yet, but I believe it's reasonably safe for the vast majority of cases. For now, the second patch is mostly meant as a proposal, and will need a review before it can be considered for a merge.
On Linux, when LISTEN_FDS is non-empty, and /run/systemd/journal/syslog is a socket, assume we are running under systemd, in which case our source shall be the journal's syslog output, instead of the default /dev/log. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- scl/system/generate-system-source.sh | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/scl/system/generate-system-source.sh b/scl/system/generate-system-source.sh index 701e238..708a7a4 100755 --- a/scl/system/generate-system-source.sh +++ b/scl/system/generate-system-source.sh @@ -43,8 +43,12 @@ osversion=${UNAME_R:-`uname -r`} case $os in Linux) + DEVLOG="/dev/log" + if [ ! -z "$LISTEN_FDS" ] && [ -S "/run/systemd/journal/syslog" ]; then + DEVLOG="/run/systemd/journal/syslog" + fi cat <<EOF -unix-dgram("/dev/log"); +unix-dgram("${DEVLOG}"); file("/proc/kmsg" program-override("kernel") flags(kernel)); EOF ;; -- 1.7.9.1
When the journal was introduced in systemd, it started to pass /run/systemd/journal/syslog to syslogds (it was passing down /dev/log before). Since we verify that the FD to open matches the expected filename, we didn't pick up the fd due to the filename mismatch. This can be fixed on the system() source level, where it can decide which to generate into the config. However, for setups that do not use system() for one reason or the other, this does not work. For them, the best solution is to try and check the passed FDs with /run/systemd/journal/syslog as the filename, so if all else fails, we pick that up. This patch does just that. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- modules/afsocket/afunix.c | 51 ++++++++++++++++++++++++++++++-------------- 1 files changed, 35 insertions(+), 16 deletions(-) diff --git a/modules/afsocket/afunix.c b/modules/afsocket/afunix.c index 8145f1a..dff1efe 100644 --- a/modules/afsocket/afunix.c +++ b/modules/afsocket/afunix.c @@ -71,8 +71,9 @@ afunix_sd_set_perm(LogDriver *s, gint perm) } #if ENABLE_SYSTEMD -static gboolean -afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) +static gint +afunix_sd_acquire_socket_with_fn(AFSocketSourceDriver *s, gint *result_fd, + const gchar *filename) { AFUnixSourceDriver *self = (AFUnixSourceDriver *) s; gint fd, fds; @@ -82,7 +83,7 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) fds = sd_listen_fds(0); if (fds == 0) - return TRUE; + return 1; msg_debug("Systemd socket activation", evt_tag_int("systemd-sockets", fds), @@ -94,19 +95,19 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) { msg_error("Failed to acquire systemd sockets, incorrectly set LISTEN_FDS environment variable?", NULL); - return FALSE; + return -1; } else if (fds > 0) { for (fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + fds; fd++) { /* check if any type is available */ - if (sd_is_socket_unix(fd, 0, -1, self->filename, 0)) + if (sd_is_socket_unix(fd, 0, -1, filename, 0)) { int type = (self->super.flags & AFSOCKET_STREAM) ? SOCK_STREAM : SOCK_DGRAM; /* check if it matches our idea of the socket type */ - if (sd_is_socket_unix(fd, type, -1, self->filename, 0)) + if (sd_is_socket_unix(fd, type, -1, filename, 0)) { *result_fd = fd; break; @@ -114,11 +115,11 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) else { msg_error("The systemd supplied UNIX domain socket is of a different type, check the configured driver and the matching systemd unit file", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", fd), evt_tag_str("expecting", type == SOCK_STREAM ? "unix-stream()" : "unix-dgram()"), NULL); - return FALSE; + return -1; } } else @@ -130,31 +131,49 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) */ msg_debug("Ignoring systemd supplied fd as it is not a UNIX domain socket", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", fd), NULL); } } } else - return TRUE; + return 1; if (*result_fd != -1) { g_fd_set_nonblock(*result_fd, TRUE); g_fd_set_cloexec(*result_fd, TRUE); msg_verbose("Acquired systemd socket", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", *result_fd), NULL); + return 1; } - else + msg_debug("Failed to acquire systemd socket, trying to open ourselves", + evt_tag_str("filename", filename), + NULL); + return 0; +} + +static gboolean +afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) +{ + AFUnixSourceDriver *self = (AFUnixSourceDriver *) s; + int status; + + status = afunix_sd_acquire_socket_with_fn(s, result_fd, self->filename); + if (status == 0) { - msg_debug("Failed to acquire systemd socket, trying to open ourselves", - evt_tag_str("filename", self->filename), - NULL); + status = afunix_sd_acquire_socket_with_fn(s, result_fd, "/run/systemd/journal/syslog"); + if (status == 1) + { + g_free(self->filename); + self->filename = g_strdup("/run/systemd/journal/syslog"); + return TRUE; + } } - return TRUE; + return !!(status == 1); } #else -- 1.7.9.1
Gergely Nagy <algernon@balabit.hu> writes:
When the journal was introduced in systemd, it started to pass /run/systemd/journal/syslog to syslogds (it was passing down /dev/log before). Since we verify that the FD to open matches the expected filename, we didn't pick up the fd due to the filename mismatch.
This can be fixed on the system() source level, where it can decide which to generate into the config. However, for setups that do not use system() for one reason or the other, this does not work.
For them, the best solution is to try and check the passed FDs with /run/systemd/journal/syslog as the filename, so if all else fails, we pick that up. This patch does just that.
A small note:
+static gboolean +afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) +{ + AFUnixSourceDriver *self = (AFUnixSourceDriver *) s; + int status; + + status = afunix_sd_acquire_socket_with_fn(s, result_fd, self->filename); + if (status == 0) { - msg_debug("Failed to acquire systemd socket, trying to open ourselves", - evt_tag_str("filename", self->filename), - NULL); + status = afunix_sd_acquire_socket_with_fn(s, result_fd, "/run/systemd/journal/syslog"); + if (status == 1) + { + g_free(self->filename); + self->filename = g_strdup("/run/systemd/journal/syslog"); + return TRUE; + } } - return TRUE; + return !!(status == 1); }
This part should not blindly call _acquire_socket_with_fn with the filename replaced, but only do that when the current self->filename is /dev/log, otherwise we might end up replacing another unix-dgram. With an extra strcmp(), the result should be safe. A bit of cleaning up around the msg_debugs() will still be in order, though. I'll try to do both tomorrow, and send an updated patch. -- |8]
On Fri, 2012-04-20 at 20:00 +0200, Gergely Nagy wrote:
When the journal was introduced in systemd, it started to pass /run/systemd/journal/syslog to syslogds (it was passing down /dev/log before). Since we verify that the FD to open matches the expected filename, we didn't pick up the fd due to the filename mismatch.
This can be fixed on the system() source level, where it can decide which to generate into the config. However, for setups that do not use system() for one reason or the other, this does not work.
For them, the best solution is to try and check the passed FDs with /run/systemd/journal/syslog as the filename, so if all else fails, we pick that up. This patch does just that.
Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- modules/afsocket/afunix.c | 51 ++++++++++++++++++++++++++++++-------------- 1 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/modules/afsocket/afunix.c b/modules/afsocket/afunix.c index 8145f1a..dff1efe 100644 --- a/modules/afsocket/afunix.c +++ b/modules/afsocket/afunix.c @@ -71,8 +71,9 @@ afunix_sd_set_perm(LogDriver *s, gint perm) }
#if ENABLE_SYSTEMD -static gboolean -afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) +static gint +afunix_sd_acquire_socket_with_fn(AFSocketSourceDriver *s, gint *result_fd, + const gchar *filename) { AFUnixSourceDriver *self = (AFUnixSourceDriver *) s; gint fd, fds;
I've seen that you also queued another round of this patch, so not applying now. Also, can you please give a better name to this function? Something like afunix_sd_acquire_named_socket() the "with_fn" part is difficult to understand IMHO.
@@ -82,7 +83,7 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) fds = sd_listen_fds(0);
if (fds == 0) - return TRUE; + return 1;
msg_debug("Systemd socket activation", evt_tag_int("systemd-sockets", fds), @@ -94,19 +95,19 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) { msg_error("Failed to acquire systemd sockets, incorrectly set LISTEN_FDS environment variable?", NULL); - return FALSE; + return -1; } else if (fds > 0) { for (fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + fds; fd++) { /* check if any type is available */ - if (sd_is_socket_unix(fd, 0, -1, self->filename, 0)) + if (sd_is_socket_unix(fd, 0, -1, filename, 0)) { int type = (self->super.flags & AFSOCKET_STREAM) ? SOCK_STREAM : SOCK_DGRAM;
/* check if it matches our idea of the socket type */ - if (sd_is_socket_unix(fd, type, -1, self->filename, 0)) + if (sd_is_socket_unix(fd, type, -1, filename, 0)) { *result_fd = fd; break; @@ -114,11 +115,11 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) else { msg_error("The systemd supplied UNIX domain socket is of a different type, check the configured driver and the matching systemd unit file", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", fd), evt_tag_str("expecting", type == SOCK_STREAM ? "unix-stream()" : "unix-dgram()"), NULL); - return FALSE; + return -1; } } else @@ -130,31 +131,49 @@ afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) */
msg_debug("Ignoring systemd supplied fd as it is not a UNIX domain socket", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", fd), NULL); } } } else - return TRUE; + return 1;
if (*result_fd != -1) { g_fd_set_nonblock(*result_fd, TRUE); g_fd_set_cloexec(*result_fd, TRUE); msg_verbose("Acquired systemd socket", - evt_tag_str("filename", self->filename), + evt_tag_str("filename", filename), evt_tag_int("systemd-sock-fd", *result_fd), NULL); + return 1; } - else + msg_debug("Failed to acquire systemd socket, trying to open ourselves", + evt_tag_str("filename", filename), + NULL); + return 0; +} + +static gboolean +afunix_sd_acquire_socket(AFSocketSourceDriver *s, gint *result_fd) +{ + AFUnixSourceDriver *self = (AFUnixSourceDriver *) s; + int status; + + status = afunix_sd_acquire_socket_with_fn(s, result_fd, self->filename); + if (status == 0) { - msg_debug("Failed to acquire systemd socket, trying to open ourselves", - evt_tag_str("filename", self->filename), - NULL); + status = afunix_sd_acquire_socket_with_fn(s, result_fd, "/run/systemd/journal/syslog"); + if (status == 1) + { + g_free(self->filename); + self->filename = g_strdup("/run/systemd/journal/syslog"); + return TRUE; + } } - return TRUE; + return !!(status == 1);
Maybe it's just me, but I have difficulties remembering magic values like 0, -1 or 1 to indicate status, especially the return statement is difficult to comprehend. I tend to indicate success with a mere boolean and if further information is needed, return them through parameters. FALSE could mean that an error occurred, TRUE with "listen_fd == -1" means there was no error but no socket could be acquired. OR, you could use an enum to name the respective return values. -- Bazsi
Balazs Scheidler <bazsi@balabit.hu> writes:
I've seen that you also queued another round of this patch, so not applying now.
Also, can you please give a better name to this function? Something like
afunix_sd_acquire_named_socket()
the "with_fn" part is difficult to understand IMHO.
Done. [...]
Maybe it's just me, but I have difficulties remembering magic values like 0, -1 or 1 to indicate status, especially the return statement is difficult to comprehend.
I tend to indicate success with a mere boolean and if further information is needed, return them through parameters.
FALSE could mean that an error occurred, TRUE with "listen_fd == -1" means there was no error but no socket could be acquired.
Also done, I went with a boolean return value, and checking *result_fd. I also made it so that the replacement only happens when the @version in the config is 3.4 or older, and syslog-ng emits a warning urging to upgrade to using the system() source instead. Post-3.4, it will not emit that warning, nor will it do the automatic replacement, but will try to open whatever it is told to, and emit a different warning ("Failed to acquire systemd socket") if needed. Updated patch will arrive shortly to the list, and is also available on my feature/3.3/systemd/systemd-source branch. Since systemd is being used more and more, I think it would be important to get this into 3.3 aswell, despite the slight functionality change. -- |8]
participants (2)
-
Balazs Scheidler
-
Gergely Nagy