Re: [PATCH] log: add not fatal spice_return function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]