On Fri, Nov 20, 2015 at 05:03:25PM +0100, Christophe Fergeau wrote: > On Fri, Nov 20, 2015 at 04:26:22PM +0100, Francois Gouget wrote: > > Does it matter? > > The client uses the g_return_xxx() functions already. Would it be a > > problem if the server did too instead of going its separate way? > > I looked at this today, one different between glib log functions and > SPICE is that the SPICE ones append the file name/line number/function > name to the logged message. glib is not doing that, which means if we > want to keep this, we'd have our own macros calling glib log > functions... > Changing spice_log to call g_log rather than doing the logging itself, > and deprecating SPICE_ABORT_LEVEL/SPICE_DEBUG_LEVEL (while making them > keep working by setting glib abort level/debug level) is quite easy. (I'll attach a wip patch to this email) > I'd love to be able to make that assumption, but I'm not sure it's 100% > true, mainly because of > http://cgit.freedesktop.org/spice/spice-common/commit/?id=c1403ee6bf4dfdd8f614f84ef145083b06a9f23e > which replaces a lot of asserts with return_if_fail(), and which does > not mention at all in the commit log whether all these places were > audited or not. Actually, Marc-André addressed this in http://lists.freedesktop.org/archives/spice-devel/2015-November/023879.html Christophe
diff --git a/common/log.c b/common/log.c
index fc5c129..c2c9755 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,6 +41,52 @@ static const char * spice_log_level_to_string(SpiceLogLevel level)
#endif
#endif
+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)
+{
+ if (debug_level == -1) {
+ 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) {
+ 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);
+ }
+ } else {
+ abort_level = SPICE_LOG_LEVEL_WARNING;
+ }
+ }
+}
+
void spice_logv(const char *log_domain,
SpiceLogLevel log_level,
const char *strloc,
@@ -75,39 +94,31 @@ void spice_logv(const char *log_domain,
const char *format,
va_list args)
{
- 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;
- }
- if (abort_level == -1) {
- abort_level = getenv("SPICE_ABORT_LEVEL") ? atoi(getenv("SPICE_ABORT_LEVEL")) : SPICE_ABORT_LEVEL_DEFAULT;
- }
+ GString *log_msg;
- if (debug_level < (int) log_level)
- return;
+ g_return_if_fail(spice_log_level_to_glib(log_level) != 0);
- fprintf(stderr, "(%s:%d): ", getenv("_"), getpid());
+ spice_log_set_debug_level();
+ spice_log_set_abort_level();
- if (log_domain) {
- fprintf(stderr, "%s-", log_domain);
- }
- if (level) {
- fprintf(stderr, "%s **: ", level);
- }
+ 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);
}
+ g_log(log_domain, spice_log_level_to_glib(log_level), "%s", log_msg->str);
+ g_string_free(log_msg, TRUE);
- fprintf(stderr, "\n");
-
+#if 0
+ //causing other issues (selinux issues as a spice_backtrace() tries to run a gdb binary, but
+ //the selinux policy prevents such things as spice-server runs in qemu context)
if (abort_level != -1 && abort_level >= (int) log_level) {
spice_backtrace();
abort();
}
+#endif
}
void spice_log(const char *log_domain,
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel