Bug 37741

Summary: Plugins have no common debug infrastructure so tuning the output level is hard
Product: Telepathy Reporter: Vivek Dasmohapatra <vivek>
Component: mission-controlAssignee: Vivek Dasmohapatra <vivek>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/vivek/telepathy-mission-control/log/?h=debug-cleanups
Whiteboard: NB#239794, r+ with trivia
i915 platform: i915 features:

Description Vivek Dasmohapatra 2011-05-30 03:36:01 UTC
MCP plugins don't have a common mechanism for controlling
debug output: This branch adds one, and enables an MCP_DEBUG
environment variable so the user can get debug from just those
classes of plugin they are interested in.
Comment 1 Simon McVittie 2011-05-30 04:01:13 UTC
+typedef enum {
+ MCP_DEBUG_NONE = 0,
+ MCP_DEBUG_ACCOUNT = 1 << 0,
+ MCP_DEBUG_ACCOUNT_STORAGE = 1 << 1,
+ MCP_DEBUG_DBUS_ACL = 1 << 2,
+ MCP_DEBUG_DISPATCH_OPERATION = 1 << 3,
+ MCP_DEBUG_DISPATCH_OPERATION_POLICY = 1 << 4,
+ MCP_DEBUG_LOADER = 1 << 5,
+ MCP_DEBUG_REQUEST = 1 << 6,
+ MCP_DEBUG_REQUEST_POLICY = 1 << 7,
+} McpDebugType;
+
+gboolean mcp_debugging (McpDebugType type);

I'd prefer mcp_is_debugging() and McpDebugFlags.

This takes an implementation detail (the set of debug domains) and makes it API - be careful with that sort of thing.

> #define DEBUG(format, ...) \
> - _mcp_debug ("%s: " format, G_STRFUNC, ##__VA_ARGS__)
> + if (mcp_debugging (MCP_DEBUG_TYPE)) \
> + g_debug ("%s: " format, G_STRLOC, ##__VA_ARGS__)

All function-like macros that expand to more than one statement should have their body wrapped in G_STMT_START { ... } G_STMT_END to avoid astonishing precedence. (If in doubt, copy from telepathy-glib...)

> +++ b/mission-control-plugins/debug.h
> +#ifdef ENABLE_DEBUG

Not in a public header, please. Public headers should never rely on anything from config.h, because anyone could #include them (not just MC itself).

In-tree plugins should get their MCP_DEBUG() (or just DEBUG() - up to you, because it shouldn't be public API anyway, for this reason) from an in-tree-only header like "mission-control-plugins/debug-internal.h".

Out-of-tree plugins should have their own debug-internal.h (per project) that does its own #ifdef, which depends whether the project providing the plugin has been compiled with debug enabled.

Rule of thumb: none of the observable functionality of a library should depend on its configure options, and its API certainly shouldn't "change shape". (It'd be OK if mcp_is_debugging() always returned FALSE if compiled without debugging, or if mcp_debug() did nothing if compiled without debugging, or something.)

> Squash a recent compiler warning
> + {

This commit shouldn't have been needed. Use G_STMT_START properly and it can be removed.
Comment 2 Simon McVittie 2011-05-30 04:19:01 UTC
> Call g_key_file_remove... rather than ...set_value when new value is NULL
> Don't call tp_intset_size on unset intset

r+ for these, please cherry-pick them.
Comment 3 Vivek Dasmohapatra 2011-06-01 08:46:20 UTC
(In reply to comment #1)

> This takes an implementation detail (the set of debug domains) and makes it API
> - be careful with that sort of thing.

Updated. I figured that all I was exposing here was one flag value for
each plugin interface (and of course the plugin interfaces have to be
exposed to the user or they can't implement them...)

I think I got all the other r- comments.
Comment 4 Vivek Dasmohapatra 2011-06-01 08:46:51 UTC
r- removed since it's up for re-review.
Comment 5 Simon McVittie 2011-06-01 09:13:35 UTC
> plugin debug header should be included via the main plugin header

If this is your intention, please make including it directly just fail.

> +#include <mission-control-plugins/mission-control-plugins.h>
> +#include <mission-control-plugins/debug.h>
> +#include <config.h>

config.h should always come first, and debug.h is now redundant.

Neither of those is critical.
Comment 6 Vivek Dasmohapatra 2011-06-08 10:32:02 UTC
Amended as per final comment and merged.
Comment 7 Will Thompson 2011-08-31 07:16:43 UTC
2375 % git tag --contains=0cfe51094842
telepathy-mission-control-5.8.0
telepathy-mission-control-5.9.1

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.