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

[Christophe Fergeau]

On Fri, Nov 27, 2015 at 10:59:57AM -0600, Jonathon Jongsma wrote:
> > +
> > +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"));
> > +            glib_abort_level = spice_log_level_to_glib(abort_level);
> > +            if (glib_abort_level != 0) {
> > +                g_log_set_always_fatal(glib_abort_level);
> 
> 
> I don't think this will do what you are expecting. I think you need to pass a
> mask of all levels that you want to be fatal. So if you pass G_LOG_LEVEL_WARNING
> to this function, G_LOG_LEVEL_CRITICAL will not be fatal, which is unexpected.

Ah indeed, I did not test this situation, thanks!

> 
> 
> > +            }
> > +        } else {
> > +            abort_level = SPICE_ABORT_LEVEL_DEFAULT;
> > +        }
> >      }
> > +}
> >  
> > -    if (debug_level < (int) log_level)
> > -        return;
> > +static void spice_logv(const char *log_domain,
> > +                       SpiceLogLevel log_level,
> > +                       const char *strloc,
> > +                       const char *function,
> > +                       const char *format,
> > +                       va_list args)
> > +{
> > +    GString *log_msg;
> > +    static gsize logging_initialized = FALSE;
> >  
> > -    fprintf(stderr, "(%s:%d): ", getenv("_"), getpid());
> > -
> > -    if (log_domain) {
> > -        fprintf(stderr, "%s-", log_domain);
> > -    }
> > -    if (level) {
> > -        fprintf(stderr, "%s **: ", level);
> > +    g_return_if_fail(spice_log_level_to_glib(log_level) != 0);
> > +    if (g_once_init_enter(&logging_initialized)) {
> > +        spice_log_set_debug_level();
> > +        spice_log_set_abort_level();
> > +        g_once_init_leave (&logging_initialized, TRUE);
> >      }
> > +
> > +    log_msg = g_string_new(NULL);
> >      if (strloc && function) {
> > -        fprintf(stderr, "%s:%s: ", strloc, function);
> > +        g_string_append_printf(log_msg, "%s:%s: ", strloc, function);
> >      }
> >      if (format) {
> > -        vfprintf(stderr, format, args);
> > +        g_string_append_vprintf(log_msg, format, args);
> >      }
> > -
> > -    fprintf(stderr, "\n");
> > +    g_log(log_domain, spice_log_level_to_glib(log_level), "%s", log_msg
> > ->str);
> > +    g_string_free(log_msg, TRUE);
> 
> 
> It's a bit unfortunate that we need to do an extra allocation here. Not sure
> whether it will make a real difference or not. 

Having the extra allocation is not strictly required if we don't mind
not having the filename/function/line number. I kept them because in the
end, I realized we cannot simply do
#define spice_warning g_warning
but we have to have a more complicated macro. Having a more complicated
macro allows us to keep the line numbers, so I added this GString code.
I don't mind getting rid of it.

The reason for not being able to do
#define spice_warning g_warning
is because we need to set up the fallback between SPICE_*_LEVEL and
glib, and because we need to have different abort behaviours between
spice_critical/g_critical/g_return_if_fail/spice_return_if_fail.

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/20151127/6d891b2e/attachment.sig>