Created attachment 162536 [details] [review]
Allow logging only specific debug topics

While debugging a focus problem, I noticed that Mutter had exactly
the debug statements I wanted under the META_DEBUG_FOCUS topic.
However, calling meta_set_verbose (true) results in enormous amounts
of other messages, and it's inconvenient to filter after having
started mutter.

This patch allows one to call Meta.add_debug_topic(Meta.DebugTopic.FOCUS)
from a console, and get just what one wants.

More fine tuned logging would be great, but I am wondering if we should not implement the way G_DEBUG works ? 

(Also, is there any reason why we could not be using g_warning/g_critical, etc., for meta_bug(), meta_warning(), meta_fatal() ? We have a crude patch for this in MeeGo.)

(In reply to comment #2)
> More fine tuned logging would be great, but I am wondering if we should not
> implement the way G_DEBUG works ? 

Maybe...I think we'd have to register a separate log domain for each category, would have to do a custom log handler to filter.  Do you think this is a blocker for getting the patch in?  It'd be more intrusive for a pretty minimal gain.

> (Also, is there any reason why we could not be using g_warning/g_critical,
> etc., for meta_bug(), meta_warning(), meta_fatal() ? We have a crude patch for
> this in MeeGo.)

Don't see why not, can you attach the patch?

(In reply to comment #3)
> Maybe...I think we'd have to register a separate log domain for each category,
> would have to do a custom log handler to filter.  Do you think this is a
> blocker for getting the patch in?  It'd be more intrusive for a pretty minimal
> gain.

I think it's simpler than that; you just define GDebugKey array that matches the strings to the corresponding enum values and use g_parse_debug_string() to parse the contents of the env variable / command line parameter, so should be simple to add on the top of your patch. (I don't object to adding the meta_add/remove_debug_topic() as such, just thinking that there are advantages to supporting a debug env variable as well).

Created attachment 162547 [details] [review]
Patch to use g_warning/g_critical for logging

The patch is to show what changes would be involved; if there are no objections, I will clean up the #if 0. (For bit of background, in MeeGo we have some glib magic that allows us to log all g_warnings centrally for debugging purposes, and we wanted to take advantage of that for mutter as well.)

Review of attachment 162547 [details] [review]:

Looks fine with the below change, and changing all the #if 0 to deletions.

::: src/core/util.c
@@ +395,3 @@
   fflush (out);
+
+  g_critical ("%s", str);

In the case where we're not setting up a log handler like your MeeGo one, this would mean we'd log messages twice, no?

I'd prefer the patch to simply axe out all the mutter stderr code and just map e.g. meta_warning directly to g_warning.

Also doesn't meta_bug map to g_error, and not g_critical?  meta_bug already calls abort(), which is what g_error is defind to do.

(In reply to comment #6)
> In the case where we're not setting up a log handler like your MeeGo one, this
> would mean we'd log messages twice, no?

Yes, you are right; I also realized that the patch breaks the mutter file logging feature, i.e., without installing a log handler any external messages (e.g., Gtk, GLib) will not get logged into the file. The file logging can be quite useful, don't necessarily want to axe it, and we would then end up with a different format of the messages in the file log than would be seen in terminal -- I think on balance this is not worth messing about with, and MeeGo can just keep it's patch.

> Also doesn't meta_bug map to g_error, and not g_critical?  meta_bug already
> calls abort(), which is what g_error is defind to do.

Just for completeness, the reason that calls g_critical in the patch is so that the back trace can be printed before abort() is called.

(In reply to comment #7)
> (In reply to comment #6)
> > In the case where we're not setting up a log handler like your MeeGo one, this
> > would mean we'd log messages twice, no?
> 
> Yes, you are right; I also realized that the patch breaks the mutter file
> logging feature, i.e., without installing a log handler any external messages
> (e.g., Gtk, GLib) will not get logged into the file. The file logging can be
> quite useful, don't necessarily want to axe it, and we would then end up with a
> different format of the messages in the file log than would be seen in terminal
> -- I think on balance this is not worth messing about with, and MeeGo can just
> keep it's patch.

Hmm, well I don't care much how the messages printed on the terminal are formatted.  Is the MeeGo format *really* strange or something?

> 
> > Also doesn't meta_bug map to g_error, and not g_critical?  meta_bug already
> > calls abort(), which is what g_error is defind to do.
> 
> Just for completeness, the reason that calls g_critical in the patch is so that
> the back trace can be printed before abort() is called.

Well I think what we really want here is something like ABRT to catch and save core files (which will be created from abort()), from which backtraces can be extracted.

(In reply to comment #8)
> Hmm, well I don't care much how the messages printed on the terminal are
> formatted.  Is the MeeGo format *really* strange or something?

The main thing about the MeeGo patch is that it removes the g_log_set_handler() loop in main() (since we want the messages to pass through the glib default handler), but doing so breaks the file logging of verbose mode. The only way to consistently use g_warning would be to get rid of the file logging together with the log handler, which I am not sure we want to do, as it looses us potentially useful debugging feature for no real benefit.

> Well I think what we really want here is something like ABRT to catch and save
> core files (which will be created from abort()), from which backtraces can be
> extracted.

Yes, but you cannot do 

  g_error ();
  meta_print_backtrace ();

and expect it to work, hence,

  g_critical ();
  meta_print_backtrace ();
  abort ();

Comment on attachment 162536 [details] [review]
Allow logging only specific debug topics

> META_DEBUG_VERBOSE = 1 << 22,

I think this should be defined as -1 instead, and used on meta_set_verbose () instead of the hardcoded -1 value. 

Other than that no objections to committing this.

Comment on attachment 162547 [details] [review]
Patch to use g_warning/g_critical for logging

This can't be pushed in this form for the reasons set out in comment 8.

Attachment 162536 [details] pushed as 343474a - Allow logging only specific debug topics