[Spice-devel] [PATCH spice-common] log: Use GLib logging levels directly

[Christophe Fergeau]

On Wed, Jun 07, 2017 at 11:22:13AM +0100, Frediano Ziglio wrote:
> As we moved toward GLib logging instead of having to convert
> every time the log level from the old system to GLib use
> directly GLib constants.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  common/log.c | 35 +++++++++++++++++++++--------------
>  common/log.h | 26 +++++++++-----------------
>  2 files changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/common/log.c b/common/log.c
> index 7437d3e..14b1c1c 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -31,16 +31,24 @@
>  #include "backtrace.h"
>  
>  static int glib_debug_level = INT_MAX;
> -static int abort_level = -1;
> +static int abort_levels = -1;

I think I'd go with abort_mask rather than abort_levels.
It looks like you can initialize it to 0, and save a != 0 test in
spice_logv.

>  
> -#ifndef SPICE_ABORT_LEVEL_DEFAULT
> +#ifndef SPICE_ABORT_LEVELS_DEFAULT
>  #ifdef SPICE_DISABLE_ABORT
> -#define SPICE_ABORT_LEVEL_DEFAULT -1
> +#define SPICE_ABORT_LEVELS_DEFAULT 0
>  #else
> -#define SPICE_ABORT_LEVEL_DEFAULT SPICE_LOG_LEVEL_CRITICAL
> +#define SPICE_ABORT_LEVELS_DEFAULT (G_LOG_LEVEL_CRITICAL|G_LOG_LEVEL_ERROR)
>  #endif
>  #endif
>  
> +typedef enum {
> +    SPICE_LOG_LEVEL_ERROR,
> +    SPICE_LOG_LEVEL_CRITICAL,
> +    SPICE_LOG_LEVEL_WARNING,
> +    SPICE_LOG_LEVEL_INFO,
> +    SPICE_LOG_LEVEL_DEBUG,
> +} SpiceLogLevel;
> +
>  static GLogLevelFlags spice_log_level_to_glib(SpiceLogLevel level)
>  {
>      static const GLogLevelFlags glib_levels[] = {
> @@ -95,23 +103,23 @@ static void spice_log_set_debug_level(void)
>  
>  static void spice_log_set_abort_level(void)
>  {
> -    if (abort_level == -1) {
> +    if (abort_levels == -1) {
>          const char *abort_str = g_getenv("SPICE_ABORT_LEVEL");
>          if (abort_str != NULL) {
>              GLogLevelFlags glib_abort_level;
>  
>              /* FIXME: To be removed after enough deprecation time */
>              g_warning("Setting SPICE_ABORT_LEVEL is deprecated, use G_DEBUG instead");
> -            abort_level = atoi(abort_str);
> -            glib_abort_level = spice_log_level_to_glib(abort_level);
> +            glib_abort_level = spice_log_level_to_glib(atoi(abort_str));
>              unsigned int fatal_mask = G_LOG_FATAL_MASK;
>              while (glib_abort_level >= G_LOG_LEVEL_ERROR) {
>                  fatal_mask |= glib_abort_level;
>                  glib_abort_level >>= 1;
>              }
> +            abort_levels = fatal_mask;
>              g_log_set_fatal_mask(SPICE_LOG_DOMAIN, fatal_mask);
>          } else {
> -            abort_level = SPICE_ABORT_LEVEL_DEFAULT;
> +            abort_levels = SPICE_ABORT_LEVELS_DEFAULT;
>          }
>      }
>  }
> @@ -146,16 +154,15 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init)
>  }
>  
>  static void spice_logv(const char *log_domain,
> -                       SpiceLogLevel log_level,
> +                       GLogLevelFlags log_level,
>                         const char *strloc,
>                         const char *function,
>                         const char *format,
>                         va_list args)
>  {
>      GString *log_msg;
> -    GLogLevelFlags glib_level = spice_log_level_to_glib(log_level);
>  
> -    if ((glib_level & G_LOG_LEVEL_MASK) > glib_debug_level) {
> +    if ((log_level & G_LOG_LEVEL_MASK) > glib_debug_level) {
>          return; // do not print anything
>      }
>  
> @@ -166,17 +173,17 @@ static void spice_logv(const char *log_domain,
>      if (format) {
>          g_string_append_vprintf(log_msg, format, args);
>      }
> -    g_log(log_domain, glib_level, "%s", log_msg->str);
> +    g_log(log_domain, log_level, "%s", log_msg->str);
>      g_string_free(log_msg, TRUE);
>  
> -    if (abort_level != -1 && abort_level >= (int) log_level) {
> +    if (abort_levels != -1 && (abort_levels & log_level) != 0) {
>          spice_backtrace();
>          abort();
>      }

I'm wondering if this codepath can be hit at all, I'd expect
g_log_set_fatal_mask to cause g_log to abort before this is reached.
(not a problem with your patch ;)

Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/edef7efa/attachment.sig>