[Spice-devel] [spice-common v3 2/7] log: Use glib for logging

[Christophe Fergeau]

On Thu, Dec 17, 2015 at 03:56:38PM -0600, Jonathon Jongsma wrote:
> > +
> > +static void spice_log_set_debug_level(void)
> > +{
> > +    if (glib_debug_level == 0) {
> > +        char *debug_str = getenv("SPICE_DEBUG_LEVEL");
> > +        if (debug_str != NULL) {
> > +            int debug_level;
> > +            char *debug_env;
> > +
> > +            g_warning("Setting SPICE_DEBUG_LEVEL is deprecated, use
> > G_MESSAGES_DEBUG instead");
> > +            debug_level = atoi(getenv("SPICE_DEBUG_LEVEL"));
> 
> Out of curiosity, why are you using getenv here again instead of just using the
> local debug_str variable you already set?

Oh, I believe this was mentioned before, forgot to change it, I'll do it
now.

> 
> 
> > +            glib_debug_level = spice_log_level_to_glib(debug_level);
> > +
> > +            /* If the debug level is too high, make sure we don't try to
> > enable
> > +             * display of glib debug logs */
> > +            if (debug_level < SPICE_LOG_LEVEL_INFO)
> > +                return;
> 
> Why? I don't understand this bit.

SPICE_DEBUG_LEVEL can be set to SPICE_LOG_LEVEL_DEBUG,
SPICE_LOG_LEVEL_INFO, in which case we need to set G_MESSAGES_DEBUG
to get a similar behaviour with g_log(). However, it can also be set
to SPICE_LOG_LEVEL_WARNING, SPICE_LOG_LEVEL_CRITICAL to hide messages
which would otherwise be shown by default. When this is the case,
setting G_MESSAGES_DEBUG is not useful (even though I don't think it
would be harmful as spice_logger() would filter out the messages
anyway).

> 
> > +
> > +            /* Make sure GLib default log handler will show the debug
> > messages. Messing with
> > +             * environment variables like this is ugly, but this only happens
> > when the legacy
> > +             * SPICE_DEBUG_LEVEL is used
> > +             */
> > +            debug_env = (char *)g_getenv("G_MESSAGES_DEBUG");
> > +            if (debug_env == NULL) {
> > +                g_setenv("G_MESSAGES_DEBUG", SPICE_LOG_DOMAIN, FALSE);
> > +            } else {
> > +                debug_env = g_strconcat(debug_env, ":", SPICE_LOG_DOMAIN,
> > NULL);
> > +                g_setenv("G_MESSAGES_DEBUG", SPICE_LOG_DOMAIN, FALSE);
> > +                g_free(debug_env);
> > +            }
> > +        }
> >      }
> > +}
> > +
> > +static void spice_log_set_abort_level(void)
> > +{
> >      if (abort_level == -1) {
> > -        abort_level = getenv("SPICE_ABORT_LEVEL") ?
> > atoi(getenv("SPICE_ABORT_LEVEL")) : SPICE_ABORT_LEVEL_DEFAULT;
> > +        char *abort_str = getenv("SPICE_ABORT_LEVEL");
> > +        if (abort_str != NULL) {
> > +            GLogLevelFlags glib_abort_level;
> > +            g_warning("Setting SPICE_ABORT_LEVEL is deprecated, use G_DEBUG
> > instead");
> > +            abort_level = atoi(getenv("SPICE_ABORT_LEVEL"));
> 
> again, why not use the local variable instead of re-calling getenv()?
> 
> > 
> > +            glib_abort_level = spice_log_level_to_glib(abort_level);
> > +            if (glib_abort_level != 0) {
> > +                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;
> > +                }
> > +                g_log_set_fatal_mask(SPICE_LOG_DOMAIN, fatal_mask);
> > +            }
> > +        } else {
> > +            abort_level = SPICE_ABORT_LEVEL_DEFAULT;
> > +        }
> >      }
> > +}
> >  
> > -    if (debug_level < (int) log_level)
> > -        return;
> > +static void spice_logger(const gchar *log_domain,
> > +                         GLogLevelFlags log_level,
> > +                         const gchar *message,
> > +                         gpointer user_data)
> > +{
> > +    if (glib_debug_level != 0) {
> > +        if ((log_level & G_LOG_LEVEL_MASK) > glib_debug_level)
> > +            return; // do not print anything
> > +    }
> > +    g_log_default_handler(log_domain, log_level, message, NULL);
> > +}
> >  
> > -    fprintf(stderr, "(%s:%d): ", getenv("_"), getpid());
> > +static void spice_log_init_once(void)
> 
> inline?

If you want, dunno how much it matters.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151218/401de2ce/attachment.sig>