GNOME Bugzilla – Bug 757311
Avoid g_error() on initialization failures
Last modified: 2015-11-09 01:02:38 UTC
One of the most common things reported to retrace.fedoraproject.org is a g_error() when This patch fixes this both for X (where Clutter fails to initialize) and for Wayland (where we usually, if not always, fail before that.) The MetaLauncher patch is admittedly very non-minimal - just replacing g_error() with g_warning(); exit(1); in meta-launcher.c would have been a lot less code restructuring, but a) I didn't like scattering exits all over the code b) I felt there was a lot of code already to turn GError's into g_errors, so I did a larger patch to make meta_launcher_new() cleanly return a GError.
Created attachment 314406 [details] [review] MetaLauncher: Don't g_error() on failure g_error() is the wrong thing to do when, for example, we can't find the DRM device, since Mutter should just fail to start rather than reporting a bug into automatic bug tracking systems. Rather than trying to decipher which errors are "expected" and which not, just make all failure paths in meta_launcher_new() return a GError out to the caller - which we make exit(1).
Created attachment 314407 [details] [review] Exit, not abort, when we fail to initialize Clutter Failing to initialize Clutter isn't something it's useful to report into automatic bug tracking systems or get a backtrace for - in fact, the most common case is that DISPLAY is unset or points to a non-existent X server. So simply exit rather than calling g_error().
Review of attachment 314406 [details] [review]: FWIW, had a look at this, and it mostly looks good. One thing I suspect needs fixing, others is just nits. ::: src/backends/native/meta-backend-native.c @@ +333,3 @@ + if (priv->launcher == NULL) + { + g_warning("Can't initialize KMS backend: %s\n", error->message); Missing space before (. @@ +334,3 @@ + { + g_warning("Can't initialize KMS backend: %s\n", error->message); + exit(1); Same here. ::: src/backends/native/meta-launcher.c @@ +65,2 @@ if (sd_pid_get_session (getpid (), &session_id) < 0) return NULL; Not setting error here. @@ +355,1 @@ + g_autofree gchar *path = get_primary_gpu_path (seat_id); Isn't the coding style to put declarations on the top of the block? @@ +421,3 @@ + Login1Seat *seat_proxy = NULL; + char *seat_id = NULL; + gboolean have_control = FALSE; Why not just do it the "normal" way, i.e. two labels, fail_release: login1_...release..(); fail: ... ? @@ +467,3 @@ + login1_session_call_release_control_sync (session_proxy, NULL, NULL); + g_clear_object(&session_proxy); + g_clear_object(&seat_proxy); Missing space before (.
Review of attachment 314407 [details] [review]: Seems to make sense to me.
Review of attachment 314406 [details] [review]: Thanks for the careful review! ::: src/backends/native/meta-launcher.c @@ +355,1 @@ + g_autofree gchar *path = get_primary_gpu_path (seat_id); You mean have all declarations at the top of the block without any substantive assignments to them? There are certainly some parts of Mutter written that way, but not all of them - meta-launcher.c itself is somewhat mixed in this regard. In general, I think with g_autoptr()/g_autofree() it's better to combine the declaration and initialization - to avoid any possibility of running the cleanup function on an uninitialized variable (as long as there are no gotos - http://blog.fishsoup.net/2015/11/05/attributecleanup-mixed-declarations-and-code-and-goto/ goes into detail) @@ +421,3 @@ + Login1Seat *seat_proxy = NULL; + char *seat_id = NULL; + gboolean have_control = FALSE; I think multiple error labels is error prone so I'd rather avoid when possible - even though it's just marginally more efficient.
Pushed to gnome-3-18 and master Attachment 314406 [details] pushed as 7fb3ecc - MetaLauncher: Don't g_error() on failure Attachment 314407 [details] pushed as 3ec3cc2 - Exit, not abort, when we fail to initialize Clutter
(In reply to Owen Taylor from comment #5) > Review of attachment 314406 [details] [review] [review]: > > Thanks for the careful review! > > ::: src/backends/native/meta-launcher.c > @@ +355,1 @@ > + g_autofree gchar *path = get_primary_gpu_path (seat_id); > > You mean have all declarations at the top of the block without any > substantive assignments to them? There are certainly some parts of Mutter > written that way, but not all of them - meta-launcher.c itself is somewhat > mixed in this regard. > > In general, I think with g_autoptr()/g_autofree() it's better to combine the > declaration and initialization - to avoid any possibility of running the > cleanup function on an uninitialized variable (as long as there are no gotos > - > http://blog.fishsoup.net/2015/11/05/attributecleanup-mixed-declarations-and- > code-and-goto/ goes into detail) I meant NULL initialized in the beginning of the block, and assigned where you now declare it. > > @@ +421,3 @@ > + Login1Seat *seat_proxy = NULL; > + char *seat_id = NULL; > + gboolean have_control = FALSE; > > I think multiple error labels is error prone so I'd rather avoid when > possible - even though it's just marginally more efficient. What I cared about was consistency and less state, not efficiency.