[syslog-ng] [PATCH 2/2] afsocket: Support a smooth upgrade path to recent systemd

[Balazs Scheidler]

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 at 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