GNOME Bugzilla – Bug 622441
Remove custom log handler
Last modified: 2021-07-05 13:49:02 UTC
Mutter had a lot of custom log handling built up from ye olde metacity day. Notably, back when operating systems didn't come with built-in crash catching frameworks, application and component authors had to roll their own, complete with backtrace() functions. The main idea with this patch is that plugins should be in control of logging. We keep meta_warning rather than doing a big search+replace substitution with g_warning, but that may still happen in the future. For GNOME Shell, we want to make at least GObject criticals fatal, since we want the OS to collect backtraces.
Created attachment 164348 [details] [review] Remove custom log handler
Discussed this some with Colin in person earlier this week. My opinion was that: * Keeping the ability to redirect logging to a file is important - I personally don't make much use of MUTTER_VERBOSE/MUTTER_DEBUG, but apparently they have been quite useful to people tracking down window manager * Having that log file have all messages is important - you want to be able to correlate warnings, etc, with the verbose messages. * It's a feature that all Mutter logging goes through g_logv() rather than bypassing it. Not only does that get everything properly interleaved and in the same place, it means that if we switch to making Mutter a "libmutter", then the disposition of logging done by Mutter can easily be controlled by the application in the standard fashion. * Logging should be set up as early as possible; by the time that plugins are loaded, it's too late. So, until we switch Mutter over to being libmutter, I think it's appropriate for control logging. I wouldn't mind removing the backtrace printing if it's causing problems - though it's not completely clear to me why it would cause problems - we aren't catching SEGV, we aren't forking off gdb as a separate process, so it shouldn't interfere with operating system crash catchers. Colin had a concern that the Mutter log handler was causing his patch in Bug 622442 - Make GObject criticals fatal not to work, or was causing G_DEBUG=fatal-warnings not to work. I don't see any reason that either would be the case. By default, Mutter doesn't touch the fatal mask. (So, I don't see why MUTTER_G_FATAL_WARNINGS is needed - G_DEBUG=fatal-warnings should be sufficient.)
Resolving WONTFIX based upon my commentary and lack of followup.
(In reply to comment #2) > * It's a feature that all Mutter logging goes through g_logv() rather than > bypassing it. Not only does that get everything properly interleaved and in the > same place, it means that if we switch to making Mutter a "libmutter", then the > disposition of logging done by Mutter can easily be controlled by the > application in the standard fashion. That would be a feature, but that's the exact opposite of how it actually works: mutter redirects everyone else's g_logv() output to go through meta_warning(), which then just uses stdio. Now that we have libmutter, I think this patch is right (or at least, on the road to right); all the mutter logging stuff should be going through g_logv, and then gnome-shell can intercept it and do whatever it feels is right with it (including logging to a file, etc, like mutter can do now).
Review of attachment 164348 [details] [review]: - I think there *does* need to be consistent handling with respect to log files if we offer MUTTER_USE_LOGFILE=1, it really should catch meta_warning()/meta_fatal() - This is clearly not right with respect to: static void log_handler (const gchar *log_domain, GLogLevelFlags log_level, const gchar *message, gpointer user_data) { meta_warning ("Log level %d: %s\n", log_level, message); meta_print_backtrace (); } const gchar *log_domains[] = { NULL, G_LOG_DOMAIN, "Gtk", "Gdk", "GLib", "Pango", "GLib-GObject", "GThread" }; for (i=0; i<G_N_ELEMENTS(log_domains); i++) g_log_set_handler (log_domains[i], G_LOG_LEVEL_MASK | G_LOG_FLAG_FATAL | G_LOG_FLAG_RECURSION, log_handler, NULL); In meta_run() - to take Gtk messages, redirect them to meta_warning, then spit meta_warning() back out to GLog with a different domain I don't think we can get rid of the MUTTER_DEBUG/MUTTER_VERBOSE envvars - there's just too much verbosity there without them to feed into g_debug() by default, but if we wanted to move MUTTER_USE_LOGFILE into the mutter binary and add GNOME_SHELL_USE_LOGFILE in the gnome-shell code, and make them log everything going through g_log() into the log file, would be fine with that if people think it's the right thing to do. ::: src/core/util.c @@ +414,2 @@ va_start (args, format); + g_logv (G_LOG_DOMAIN, G_LOG_LEVEL_ERROR, format, args); Drops a core file, is not supposed to
Assigning to danw - logging triggers flashbacks to log4j if I look at it too much
Review of attachment 164348 [details] [review]: Oh. didn't notice going back to this patch that it actually killed the handling in main.c as well. In any case, this needs work to make it consistent and not regress the ability to use all the myriad debug statements strewn throughout mutter.
Comment on attachment 164348 [details] [review] Remove custom log handler >For GNOME Shell, we want to make at least GObject criticals fatal, >since we want the OS to collect backtraces. I'm not sure that's agreed upon, and it doesn't need to be in the mutter commit message anyway. > meta_bug (const char *format, ...) >- if (no_prefix == 0) >- utf8_fputs (_("Bug in window manager: "), out); You lost this part, which I think is important (in the meta_bug() case). You could just prepend that to the format string. > abort (); G_LOG_LEVEL_ERROR is already fatal by default, so we don't need that any more. (And if the user chooses to make ERROR non-fatal, then we don't need to position ourselves between the gun and their foot.) What about meta_topic? It seems like that should be turned into G_LOG_LEVEL_DEBUG. And then you can get rid of ensure_logfile() and utf8_fputs(), and then meta_push_no_msg_prefix() and meta_pop_no_msg_prefix() become unnecessary too. As Owen says, the MUTTER_USE_LOGFILE in ensure_logfile() should be implemented in core/mutter.c instead.
*** Bug 707421 has been marked as a duplicate of this bug. ***
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.