[PATCH 2/5] log: Refactor log level text output

[Aleksander Morgado]

Hey Torsten,

On 22/05/17 07:49, Torsten Hilbrich wrote:
> Adding a helper function to turn the log level into the text. Also
> add an internal flag to disable this output. This flag will become
> useful when adding systemd journal support.

See some comments below.

> ---
>  src/mm-log.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/src/mm-log.c b/src/mm-log.c
> index e0255c3..ec622dc 100644
> --- a/src/mm-log.c
> +++ b/src/mm-log.c
> @@ -48,6 +48,7 @@ static guint32 log_level = LOGL_INFO | LOGL_WARN | LOGL_ERR;
>  static GTimeVal rel_start = { 0, 0 };
>  static int logfd = -1;
>  static gboolean func_loc = FALSE;
> +static gboolean append_log_level_text = TRUE;
>  
>  typedef struct {
>      guint32 num;
> @@ -81,6 +82,24 @@ mm_to_syslog_priority(int level)
>      return LOG_INFO;
>  }
>  
> +static const char *
> +log_level_description(int level)

A whitespace is missing before the "(". As in the previous patch, we could pass a "MMLogLevel level" instead of a generic int.

> +{
> +    switch (level) {
> +    case LOGL_DEBUG:
> +        return "debug";
> +    case LOGL_WARN:
> +        return "warn";
> +    case LOGL_INFO:
> +        return "info";
> +    case LOGL_ERR:
> +        return "error";
> +    }
> +    /* this should not happen as all valid enum values are supported
> +       above */
> +    return "unknown";

How about this better?
  g_assert_not_reached();
  return NULL;

> +}
> +
>  void
>  _mm_log (const char *loc,
>           const char *func,
> @@ -101,20 +120,8 @@ _mm_log (const char *loc,
>      } else
>          g_string_truncate (msgbuf, 0);
>  
> -    switch (level) {
> -    case LOGL_DEBUG:
> -        g_string_append (msgbuf, "<debug> ");
> -        break;
> -    case LOGL_INFO:
> -        g_string_append (msgbuf, "<info>  ");
> -        break;
> -    case LOGL_WARN:
> -        g_string_append (msgbuf, "<warn>  ");
> -        break;
> -    case LOGL_ERR:
> -        g_string_append (msgbuf, "<error> ");
> -        break;

Before this patch all the level message strings were aligned; e.g.:

 <info>  something
 <debug> something else

The <info> and <warn> items had an extra whitespace to have that. Can we still have that with this cleanup? We could just have log_level_description() return the full "<xxxx> " string, with trailing whitespaces (1 or 2) included.

> -    }
> +    if (append_log_level_text)
> +        g_string_append_printf (msgbuf, "<%s> ", log_level_description(level));

A whitespace is missing before the "("


>      if (ts_flags == TS_FLAG_WALL) {
>          g_get_current_time (&tv);
> 


-- 
Aleksander
https://aleksander.es