[PATCH 1/5] log: Refactor evaluation of log level

[Aleksander Morgado]

Hey Torsten,

On 22/05/17 07:49, Torsten Hilbrich wrote:
> The level was checked twice in _mm_log. Once at the beginning and
> again when turning the level into both the syslog priority and some
> descriptive text which was added to the log level. Removing the check
> at the second location as it was redundant.
> 
> Also adding a helper function to turn the mm log level into the syslog
> equivalent. This will become handy when adding support for systemd
> journal.

See some minor comments below.

> ---
>  src/mm-log.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mm-log.c b/src/mm-log.c
> index bac0fce..e0255c3 100644
> --- a/src/mm-log.c
> +++ b/src/mm-log.c
> @@ -65,6 +65,22 @@ static const LogDesc level_descs[] = {
>  static GString *msgbuf = NULL;
>  static volatile gsize msgbuf_once = 0;
>  
> +static int
> +mm_to_syslog_priority(int level)

A whitespace is missing before the "(".

Also, I'm thinking we could actually setup a typedef enum for the MM logging level, something like 

typedef enum {
    MM_LOG_LEVEL_ERR   = 0x00000001,
    MM_LOG_LEVEL_WARN  = 0x00000002,
    MM_LOG_LEVEL_INFO  = 0x00000004,
    MM_LOG_LEVEL_DEBUG = 0x00000008
} MMLogLevel;

What do you think?

I'm suggesting this because this would let us pass "MMLogLevel level" in this new method, which the compiler will treat in a more strict way when checking for missing items in the switch.

> +{
> +    switch (level) {
> +    case LOGL_DEBUG:
> +        return LOG_DEBUG;
> +    case LOGL_WARN:
> +        return LOG_WARNING;
> +    case LOGL_INFO:
> +        return LOG_INFO;
> +    case LOGL_ERR:
> +        return LOG_ERR;
> +    }
> +    return LOG_INFO;

And if we add the new typedef, this fallback return LOG_INFO would also go away, and we can set here g_assert_not_reached().

> +}
> +
>  void
>  _mm_log (const char *loc,
>           const char *func,
> @@ -74,7 +90,6 @@ _mm_log (const char *loc,
>  {
>      va_list args;
>      GTimeVal tv;
> -    int syslog_priority = LOG_INFO;
>      ssize_t ign;
>  
>      if (!(log_level & level))
> @@ -86,18 +101,20 @@ _mm_log (const char *loc,
>      } else
>          g_string_truncate (msgbuf, 0);
>  
> -    if ((log_level & LOGL_DEBUG) && (level == LOGL_DEBUG))
> +    switch (level) {
> +    case LOGL_DEBUG:
>          g_string_append (msgbuf, "<debug> ");
> -    else if ((log_level & LOGL_INFO) && (level == LOGL_INFO))
> +        break;
> +    case LOGL_INFO:
>          g_string_append (msgbuf, "<info>  ");
> -    else if ((log_level & LOGL_WARN) && (level == LOGL_WARN)) {
> +        break;
> +    case LOGL_WARN:
>          g_string_append (msgbuf, "<warn>  ");
> -        syslog_priority = LOG_WARNING;
> -    } else if ((log_level & LOGL_ERR) && (level == LOGL_ERR)) {
> +        break;
> +    case LOGL_ERR:
>          g_string_append (msgbuf, "<error> ");
> -        syslog_priority = LOG_ERR;
> -    } else
> -        return;
> +        break;
> +    }
>  
>      if (ts_flags == TS_FLAG_WALL) {
>          g_get_current_time (&tv);
> @@ -127,7 +144,7 @@ _mm_log (const char *loc,
>      g_string_append_c (msgbuf, '\n');
>  
>      if (logfd < 0)
> -        syslog (syslog_priority, "%s", msgbuf->str);
> +        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 */
> 


-- 
Aleksander
https://aleksander.es