add zoned debug support

PATCH RESEND In review Andres Salomon 2 participants 2 messages Aug 9, 2009 – Aug 12, 2009

Share & tools

Andres Salomon

author

2009-08-09 00:11 UTC #0 lore raw

PATCH RESEND 1/1 add zoned debug support

Unfortunately, I can't think of a way to support dynamic flag modification (other than perhaps adding a dbus method to do it).

From 6b9569c22193a1d0c8d8d6f38692d5f8f43429c9 Mon Sep 17 00:00:00 2001
From: Andres Salomon <[email protected]>
Date: Sat, 8 Aug 2009 20:06:12 -0400
Subject: [PATCH] ofono: add zoned debug support

This adds debug flags so that when users are debugging, they can pass arguments to --debug to specify what they want shown. --debug without any args defaults to prior behavior.

---
 include/log.h |   11 +++++++++--
 src/log.c     |   11 +++++++----
 src/main.c    |   25 +++++++++++++++++++++++--
 src/ofono.h   |    2 +-
 4 files changed, 40 insertions(+), 9 deletions(-)

include/log.h +9 -2

index 47e5ec8..8a82f13 100644
@@ -26,6 +26,10 @@
 extern "C" {
 #endif
 
+typedef enum {
+	OFONO_DEBUG_CORE = 1 << 0,
+} ofono_debug_flags;
+
 /**
  * SECTION:log
  * @title: Logging premitives

@@ -36,8 +40,11 @@ extern void ofono_info(const char *format, ...)
 				__attribute__((format(printf, 1, 2)));
 extern void ofono_error(const char *format, ...)
 				__attribute__((format(printf, 1, 2)));
-extern void ofono_debug(const char *format, ...)
-				__attribute__((format(printf, 1, 2)));
+extern void __ofono_debug(ofono_debug_flags flag, const char *format, ...)
+				__attribute__((format(printf, 2, 3)));
+#define ofono_debug(format, ...) \
+			__ofono_debug(OFONO_DEBUG_CORE, (format), ##__VA_ARGS__)
+
 
 /**
  * DBG:

src/log.c +7 -4

index 273e3ba..167fe21 100644
@@ -29,6 +29,7 @@
 #include "ofono.h"
 
 static volatile gboolean debug_enabled = FALSE;
+static guint debug_flags;
 
 /**
  * ofono_info:

@@ -67,7 +68,8 @@ void ofono_error(const char *format, ...)
 }
 
 /**
- * ofono_debug:
+ * __ofono_debug:
+ * @flag: zone flag (ie, OFONO_DEBUG_CORE)
  * @format: format string
  * @varargs: list of arguments
  *

@@ -76,11 +78,11 @@ void ofono_error(const char *format, ...)
  * The actual output of the debug message is controlled via a command line
  * switch. If not enabled, these messages will be ignored.
  */
-void ofono_debug(const char *format, ...)
+void __ofono_debug(ofono_debug_flags flag, const char *format, ...)
 {
 	va_list ap;
 
-	if (debug_enabled == FALSE)
+	if (!debug_enabled || !(debug_flags & flag))
 		return;
 
 	va_start(ap, format);

@@ -98,7 +100,7 @@ void __ofono_toggle_debug(void)
 		debug_enabled = TRUE;
 }
 
-int __ofono_log_init(gboolean detach, gboolean debug)
+int __ofono_log_init(gboolean detach, gboolean debug, guint dflags)
 {
 	int option = LOG_NDELAY | LOG_PID;

@@ -110,6 +112,7 @@ int __ofono_log_init(gboolean detach, gboolean debug)
 	syslog(LOG_INFO, "oFono version %s", VERSION);
 
 	debug_enabled = debug;
+	debug_flags = dflags;
 
 	return 0;
 }

src/main.c +23 -2

index 7542e13..7227bde 100644
@@ -54,12 +54,33 @@ static void system_bus_disconnected(DBusConnection *conn, void *user_data)
 
 static gboolean option_detach = TRUE;
 static gboolean option_debug = FALSE;
+static guint debug_flags = 0;
+
+static GDebugKey keys[] = {
+	{ "core", OFONO_DEBUG_CORE },
+};
+
+static gboolean parse_debug_flags(const gchar *option_name, const gchar *value,
+		gpointer data, GError **err)
+{
+	option_debug = TRUE;
+
+	/* NULL means no string was supplied to --debug.  We default to "core"
+	 * in that scenario; perhaps we should be defaulting to "all" instead? */
+	if (!value)
+		value = "core";
+
+	debug_flags = g_parse_debug_string(value, keys,
+			sizeof(keys) / sizeof(keys[0]));
+	return TRUE;
+}
 
 static GOptionEntry options[] = {
 	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
 				G_OPTION_ARG_NONE, &option_detach,
 				"Don't run as daemon in background" },
-	{ "debug", 'd', 0, G_OPTION_ARG_NONE, &option_debug,
+	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
+				G_OPTION_ARG_CALLBACK, &parse_debug_flags,
 				"Enable debug information output" },
 	{ NULL },
 };

@@ -109,7 +130,7 @@ int main(int argc, char **argv)
 	}
 #endif
 
-	__ofono_log_init(option_detach, option_debug);
+	__ofono_log_init(option_detach, option_debug, debug_flags);
 
 	dbus_error_init(&error);

src/ofono.h +1 -1

index 63f33ad..c791c2d 100644
@@ -30,7 +30,7 @@ const char **__ofono_modem_get_list();
 
 #include <ofono/log.h>
 
-int __ofono_log_init(gboolean detach, gboolean debug);
+int __ofono_log_init(gboolean detach, gboolean debug, guint dflags);
 void __ofono_log_cleanup(void);
 
 void __ofono_toggle_debug(void);

Signature · 1 line

1.6.3.3

Marcel Holtmann

2009-08-12 04:32 UTC #1 lore raw

Hi Andres,

Quoted 26 lines

This adds debug flags so that when users are debugging, they can pass arguments to --debug to specify what they want shown. --debug without any args defaults to prior behavior.

---
 include/log.h |   11 +++++++++--
 src/log.c     |   11 +++++++----
 src/main.c    |   25 +++++++++++++++++++++++--
 src/ofono.h   |    2 +-
 4 files changed, 40 insertions(+), 9 deletions(-)

include/log.h +4 -0

index 47e5ec8..8a82f13 100644
@@ -26,6 +26,10 @@
 extern "C" {
 #endif
 
+typedef enum {
+	OFONO_DEBUG_CORE = 1 << 0,
+} ofono_debug_flags;
+
 /**
  * SECTION:log
  * @title: Logging premitives

include/log.h extern void ofono_info(const char *format, ...) · line 47

Quoted 10 lines

include/log.h +5 -2

@@ -36,8 +40,11 @@ extern void ofono_info(const char *format, ...)
 				__attribute__((format(printf, 1, 2)));
 extern void ofono_error(const char *format, ...)
 				__attribute__((format(printf, 1, 2)));
-extern void ofono_debug(const char *format, ...)
-				__attribute__((format(printf, 1, 2)));
+extern void __ofono_debug(ofono_debug_flags flag, const char *format, ...)
+				__attribute__((format(printf, 2, 3)));
+#define ofono_debug(format, ...) \
+			__ofono_debug(OFONO_DEBUG_CORE, (format), ##__VA_ARGS__)
+

I don't like this. __ofono functions should not be part of public header files.

Instead of trying to workaround previous users, just fix the users and provide the default zone for DBG().

Quoted 25 lines

include/log.h +0 -0

 /**
  * DBG:

src/log.c +3 -1

index 273e3ba..167fe21 100644
@@ -29,6 +29,7 @@
 #include "ofono.h"
 
 static volatile gboolean debug_enabled = FALSE;
+static guint debug_flags;
 
 /**
  * ofono_info:

@@ -67,7 +68,8 @@ void ofono_error(const char *format, ...)
 }
 
 /**
- * ofono_debug:
+ * __ofono_debug:
+ * @flag: zone flag (ie, OFONO_DEBUG_CORE)
  * @format: format string
  * @varargs: list of arguments
  *

src/log.c void ofono_error(const char *format, ...) · line 82

Quoted 6 lines

src/log.c +1 -1

@@ -76,11 +78,11 @@ void ofono_error(const char *format, ...)
  * The actual output of the debug message is controlled via a command line
  * switch. If not enabled, these messages will be ignored.
  */
-void ofono_debug(const char *format, ...)
+void __ofono_debug(ofono_debug_flags flag, const char *format, ...)
 {

Why not just using unsigned long here. I don't like the idea of overloading enum with bitmask here.

Actually the more I think about it, do we wanna have multiple zones per debug message. That sounds way to complicated anyway. So why not just make ofono_debug_flags a simple enum and then use the bitmask only internally.

Quoted 17 lines

src/log.c +2 -2

 	va_list ap;
 
-	if (debug_enabled == FALSE)
+	if (!debug_enabled || !(debug_flags & flag))
 		return;
 
 	va_start(ap, format);

@@ -98,7 +100,7 @@ void __ofono_toggle_debug(void)
 		debug_enabled = TRUE;
 }
 
-int __ofono_log_init(gboolean detach, gboolean debug)
+int __ofono_log_init(gboolean detach, gboolean debug, guint dflags)
 {
 	int option = LOG_NDELAY | LOG_PID;

src/log.c int __ofono_log_init(gboolean detach, gboolean debug) · line 115

Quoted 4 lines

src/log.c +1 -0

@@ -110,6 +112,7 @@ int __ofono_log_init(gboolean detach, gboolean debug)
 	syslog(LOG_INFO, "oFono version %s", VERSION);
 
 	debug_enabled = debug;
+	debug_flags = dflags;

This is double. If dflags are not set, then don't enable debug. No need to provide two parameters that do the same.

Also dflags is a pretty weird variable name.

Quoted 42 lines

src/log.c +0 -0

 	return 0;
 }

src/main.c +22 -1

index 7542e13..7227bde 100644
@@ -54,12 +54,33 @@ static void system_bus_disconnected(DBusConnection *conn, void *user_data)
 
 static gboolean option_detach = TRUE;
 static gboolean option_debug = FALSE;
+static guint debug_flags = 0;
+
+static GDebugKey keys[] = {
+	{ "core", OFONO_DEBUG_CORE },
+};
+
+static gboolean parse_debug_flags(const gchar *option_name, const gchar *value,
+		gpointer data, GError **err)
+{
+	option_debug = TRUE;
+
+	/* NULL means no string was supplied to --debug.  We default to "core"
+	 * in that scenario; perhaps we should be defaulting to "all" instead? */
+	if (!value)
+		value = "core";
+
+	debug_flags = g_parse_debug_string(value, keys,
+			sizeof(keys) / sizeof(keys[0]));
+	return TRUE;
+}
 
 static GOptionEntry options[] = {
 	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
 				G_OPTION_ARG_NONE, &option_detach,
 				"Don't run as daemon in background" },
-	{ "debug", 'd', 0, G_OPTION_ARG_NONE, &option_debug,
+	{ "debug", 'd', G_OPTION_FLAG_OPTIONAL_ARG,
+				G_OPTION_ARG_CALLBACK, &parse_debug_flags,
 				"Enable debug information output" },
 	{ NULL },
 };

src/main.c int main(int argc, char **argv) · line 133

Quoted 5 lines

src/main.c +1 -1

@@ -109,7 +130,7 @@ int main(int argc, char **argv)
 	}
 #endif
 
-	__ofono_log_init(option_detach, option_debug);
+	__ofono_log_init(option_detach, option_debug, debug_flags);

Same as from above. If debug_flags = 0, then debug is disabled.

Regards

Marcel