[PATCH 4/5] log: Introduce function pointer to handle different log backends

[Aleksander Morgado]

Hey Torsten,

See comments below.

On 22/05/17 07:49, Torsten Hilbrich wrote:
> This allows for easier additions of other logging mechanism.
> ---
>  src/mm-log.c | 53 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/src/mm-log.c b/src/mm-log.c
> index bedf88f..6578d68 100644
> --- a/src/mm-log.c
> +++ b/src/mm-log.c
> @@ -50,6 +50,12 @@ static int logfd = -1;
>  static gboolean func_loc = FALSE;
>  static gboolean append_log_level_text = TRUE;
>  
> +static void (*log_backend) (const char *loc,
> +                            const char *func,
> +                            int level,

It isn't clear what kind of enum "level" is. For the looks of it in this patch, it is the syslog log level, but I believe we should stick to passing to the backend the MM specific log level (here, again a MMLogLevel enum would help). I understand that the log level isn't currently used in the file backend proposed here, but for consistency, I think it makes sense to avoid passing the syslog log level to every backend.


> +                            const char *message,
> +                            size_t length);
> +
>  typedef struct {
>      guint32 num;
>      const char *name;
> @@ -63,6 +69,28 @@ static const LogDesc level_descs[] = {
>      { 0, NULL }
>  };
>  
> +static void log_backend_file (const char *loc,
> +                              const char *func,
> +                              int level,
> +                              const char *message,
> +                              size_t length)

The "static void" part should go in its own line when the function is defined.

> +{
> +    ssize_t ign;
> +    ign = write (logfd, message, length);
> +    if (ign) {} /* whatever; really shut up about unused result */
> +
> +    fsync (logfd);  /* Make sure output is dumped to disk immediately  */
> +}
> +
> +static void log_backend_syslog (const char *loc,
> +                                const char *func,
> +                                int level,
> +                                const char *message,
> +                                size_t length)

The "static void" part should go in its own line when the function is defined.

> +{
> +    syslog (level, "%s", message);
> +}
> +
>  static GString *msgbuf = NULL;
>  static volatile gsize msgbuf_once = 0;
>  
> @@ -129,7 +157,6 @@ _mm_log (const char *loc,
>  {
>      va_list args;
>      GTimeVal tv;
> -    ssize_t ign;
>  
>      if (!(log_level & level))
>          return;
> @@ -170,14 +197,7 @@ _mm_log (const char *loc,
>  
>      g_string_append_c (msgbuf, '\n');
>  
> -    if (logfd < 0)
> -        syslog (mm_to_syslog_priority (level), "%s", msgbuf->str);
> -    else {
> -        ign = write (logfd, msgbuf->str, msgbuf->len);
> -        if (ign) {} /* whatever; really shut up about unused result */
> -
> -        fsync (logfd);  /* Make sure output is dumped to disk immediately */
> -    }
> +    log_backend (loc, func, mm_to_syslog_priority (level), msgbuf->str, msgbuf->len);
>  }
>  
>  static void
> @@ -186,14 +206,7 @@ log_handler (const gchar *log_domain,
>               const gchar *message,
>               gpointer ignored)
>  {
> -    ssize_t ign;
> -
> -    if (logfd < 0)
> -        syslog (glib_to_syslog_priority (level), "%s", message);
> -    else {
> -        ign = write (logfd, message, strlen (message));
> -        if (ign) {} /* whatever; really shut up about unused result */
> -    }
> +    log_backend (NULL, NULL, glib_to_syslog_priority (level), message, strlen(message));
>  }
>  
>  gboolean
> @@ -247,9 +260,10 @@ mm_log_setup (const char *level,
>      /* Grab start time for relative timestamps */
>      g_get_current_time (&rel_start);
>  
> -    if (log_file == NULL)
> +    if (log_file == NULL) {
>          openlog (G_LOG_DOMAIN, LOG_CONS | LOG_PID | LOG_PERROR, LOG_DAEMON);
> -    else {
> +        log_backend = log_backend_syslog;
> +    } else {
>          logfd = open (log_file,
>                        O_CREAT | O_APPEND | O_WRONLY,
>                        S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
> @@ -259,6 +273,7 @@ mm_log_setup (const char *level,
>                           errno, strerror (errno));
>              return FALSE;
>          }
> +        log_backend = log_backend_file;
>      }
>  
>      g_log_set_handler (G_LOG_DOMAIN,
> 


-- 
Aleksander
https://aleksander.es