Hi Andres,
> 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(-)
>
> diff --git a/include/log.h b/include/log.h
> index 47e5ec8..8a82f13 100644
> --- a/include/log.h
> +++ b/include/log.h
> @@ -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__)
> +
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().
> /**
> * DBG:
> diff --git a/src/log.c b/src/log.c
> index 273e3ba..167fe21 100644
> --- a/src/log.c
> +++ b/src/log.c
> @@ -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, ...)
> {
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.
> 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;
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.
> return 0;
> }
> diff --git a/src/main.c b/src/main.c
> index 7542e13..7227bde 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -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);
Same as from above. If debug_flags = 0, then debug is disabled.
Regards
Marcel
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
