[Spice-devel] [spice-common 1/7] log: Use glib for logging
Jonathon Jongsma
jjongsma at redhat.com
Fri Nov 27 08:59:57 PST 2015
On Fri, 2015-11-27 at 16:47 +0100, Christophe Fergeau wrote:
> spice-common has been duplicating glib logging methods for a long while.
> Now that spice-common is depending on glib, it's more consistent to use
> glib logging too. However, the code base is still using spice logging
> functions.
> This commit aims to make spice logging go through glib logging system,
> while keeping the same behaviour for the SPICE_ABORT_LEVEL and
> SPICE_DEBUG_LEVEL environment variables. They are deprecated however in
> favour of the glib alternatives (G_DEBUG and G_MESSAGES_DEBUG).
> ---
> common/log.c | 114 ++++++++++++++++++++++++++++++++--------------------------
> -
> common/log.h | 7 ----
> 2 files changed, 62 insertions(+), 59 deletions(-)
>
> diff --git a/common/log.c b/common/log.c
> index fc5c129..09894a6 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -1,5 +1,5 @@
> /*
> - Copyright (C) 2012 Red Hat, Inc.
> + Copyright (C) 2012-2015 Red Hat, Inc.
>
> This library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
> @@ -19,6 +19,7 @@
> #include <config.h>
> #endif
>
> +#include <glib.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <sys/types.h>
> @@ -32,34 +33,6 @@
> static int debug_level = -1;
> static int abort_level = -1;
>
> -static const char * spice_log_level_to_string(SpiceLogLevel level)
> -{
> -#ifdef _MSC_VER
> - /* MSVC++ does not implement C99 */
> - static const char *to_string[] = {
> - "ERROR",
> - "CRITICAL",
> - "Warning",
> - "Info",
> - "Debug"};
> -#else
> - static const char *to_string[] = {
> - [ SPICE_LOG_LEVEL_ERROR ] = "ERROR",
> - [ SPICE_LOG_LEVEL_CRITICAL ] = "CRITICAL",
> - [ SPICE_LOG_LEVEL_WARNING ] = "Warning",
> - [ SPICE_LOG_LEVEL_INFO ] = "Info",
> - [ SPICE_LOG_LEVEL_DEBUG ] = "Debug",
> - };
> -#endif
> - const char *str = NULL;
> -
> - if (level < SPICE_N_ELEMENTS(to_string)) {
> - str = to_string[level];
> - }
> -
> - return str;
> -}
> -
> #ifndef SPICE_ABORT_LEVEL_DEFAULT
> #ifdef SPICE_DISABLE_ABORT
> #define SPICE_ABORT_LEVEL_DEFAULT -1
> @@ -68,41 +41,78 @@ static const char *
> spice_log_level_to_string(SpiceLogLevel level)
> #endif
> #endif
>
> -void spice_logv(const char *log_domain,
> - SpiceLogLevel log_level,
> - const char *strloc,
> - const char *function,
> - const char *format,
> - va_list args)
> +static GLogLevelFlags spice_log_level_to_glib(SpiceLogLevel level)
> +{
> + static GLogLevelFlags glib_levels[] = {
> + [ SPICE_LOG_LEVEL_ERROR ] = G_LOG_LEVEL_ERROR,
> + [ SPICE_LOG_LEVEL_CRITICAL ] = G_LOG_LEVEL_CRITICAL,
> + [ SPICE_LOG_LEVEL_WARNING ] = G_LOG_LEVEL_WARNING,
> + [ SPICE_LOG_LEVEL_INFO ] = G_LOG_LEVEL_INFO,
> + [ SPICE_LOG_LEVEL_DEBUG ] = G_LOG_LEVEL_DEBUG,
> + };
> + g_return_val_if_fail ((level >= 0) || (level <
> G_N_ELEMENTS(glib_levels)), 0);
> +
> + return glib_levels[level];
> +}
> +
> +static void spice_log_set_debug_level(void)
> {
> - const char *level = spice_log_level_to_string(log_level);
> -
> if (debug_level == -1) {
> - debug_level = getenv("SPICE_DEBUG_LEVEL") ?
> atoi(getenv("SPICE_DEBUG_LEVEL")) : SPICE_LOG_LEVEL_WARNING;
> + char *debug_str = getenv("SPICE_DEBUG_LEVEL");
> + if (debug_str != NULL) {
> + g_warning("Setting SPICE_DEBUG_LEVEL is deprecated, use
> G_MESSAGES_DEBUG instead");
> + debug_level = atoi(getenv("SPICE_DEBUG_LEVEL"));
> + g_setenv("G_MESSAGES_DEBUG", "all", FALSE);
> + } else {
> + debug_level = SPICE_LOG_LEVEL_WARNING;
> + }
> }
> +}
> +
> +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.
> + }
> + } 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.
>
> if (abort_level != -1 && abort_level >= (int) log_level) {
> spice_backtrace();
> diff --git a/common/log.h b/common/log.h
> index d9e6023..f336909 100644
> --- a/common/log.h
> +++ b/common/log.h
> @@ -41,13 +41,6 @@ typedef enum {
> SPICE_LOG_LEVEL_DEBUG,
> } SpiceLogLevel;
>
> -void spice_logv(const char *log_domain,
> - SpiceLogLevel log_level,
> - const char *strloc,
> - const char *function,
> - const char *format,
> - va_list args) SPICE_ATTR_PRINTF(5, 0);
> -
> void spice_log(const char *log_domain,
> SpiceLogLevel log_level,
> const char *strloc,
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
More information about the Spice-devel
mailing list