From 3803fd9511eb5341412507fa706f8098a37e4897 Mon Sep 17 00:00:00 2001 From: Giovanni Campagna Date: Mon, 12 Aug 2013 09:46:07 +0200 Subject: [PATCH] wayland: don't use fork() and SIGCHLD to spawn processes It is a very bad idea in a glib program (especially one heavily using glib child watching facilities, like gnome-shell) to handle SIGCHLD. While we're there, let's also use g_spawn_async, which solves some malloc-after-fork problems and makes the code generally cleaner. https://bugzilla.gnome.org/show_bug.cgi?id=705816 --- src/core/main.c | 15 ---- src/wayland/meta-wayland-private.h | 2 - src/wayland/meta-wayland.c | 24 ------ src/wayland/meta-xwayland.c | 123 +++++++++++++++++++---------- 4 files changed, 80 insertions(+), 84 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 276dba582..0326d8ec3 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -368,9 +368,6 @@ signal_handler (int signum) case SIGTERM: write (signal_pipe_fds[1], "T", 1); break; - case SIGCHLD: - write (signal_pipe_fds[1], "C", 1); - break; default: break; } @@ -407,11 +404,6 @@ on_signal (GIOChannel *source, case 'T': /* SIGTERM */ meta_quit (META_EXIT_SUCCESS); break; -#ifdef HAVE_WAYLAND - case 'C': /* SIGCHLD */ - meta_wayland_handle_sig_child (); - break; -#endif default: g_warning ("Spurious character '%c' read from signal pipe", signal); } @@ -460,13 +452,6 @@ meta_init (void) g_printerr ("Failed to register SIGTERM handler: %s\n", g_strerror (errno)); - if (meta_is_wayland_compositor ()) - { - if (sigaction (SIGCHLD, &act, NULL) < 0) - g_printerr ("Failed to register SIGCHLD handler: %s\n", - g_strerror (errno)); - } - if (g_getenv ("MUTTER_VERBOSE")) meta_set_verbose (TRUE); if (g_getenv ("MUTTER_DEBUG")) diff --git a/src/wayland/meta-wayland-private.h b/src/wayland/meta-wayland-private.h index 92f5d83e1..a859378c1 100644 --- a/src/wayland/meta-wayland-private.h +++ b/src/wayland/meta-wayland-private.h @@ -349,8 +349,6 @@ void meta_wayland_finalize (void); * API after meta_wayland_init() has been called. */ MetaWaylandCompositor *meta_wayland_compositor_get_default (void); -void meta_wayland_handle_sig_child (void); - void meta_wayland_compositor_repick (MetaWaylandCompositor *compositor); void meta_wayland_compositor_set_input_focus (MetaWaylandCompositor *compositor, diff --git a/src/wayland/meta-wayland.c b/src/wayland/meta-wayland.c index 7c41c217d..159499e1c 100644 --- a/src/wayland/meta-wayland.c +++ b/src/wayland/meta-wayland.c @@ -1576,27 +1576,3 @@ meta_wayland_finalize (void) { meta_xwayland_stop (meta_wayland_compositor_get_default ()); } - -void -meta_wayland_handle_sig_child (void) -{ - int status; - pid_t pid = waitpid (-1, &status, WNOHANG); - MetaWaylandCompositor *compositor = &_meta_wayland_compositor; - - /* The simplest measure to avoid infinitely re-spawning a crashing - * X server */ - if (pid == compositor->xwayland_pid) - { - if (!WIFEXITED (status)) - g_critical ("X Wayland crashed; aborting"); - else - { - /* For now we simply abort if we see the server exit. - * - * In the future X will only be loaded lazily for legacy X support - * but for now it's a hard requirement. */ - g_critical ("Spurious exit of X Wayland server"); - } - } -} diff --git a/src/wayland/meta-xwayland.c b/src/wayland/meta-xwayland.c index 7ff30bf92..56d3b7152 100644 --- a/src/wayland/meta-xwayland.c +++ b/src/wayland/meta-xwayland.c @@ -28,6 +28,8 @@ #include #include #include +#include +#include static char * create_lockfile (int display, int *display_out) @@ -183,6 +185,39 @@ bind_to_unix_socket (int display) return fd; } +static void +uncloexec_and_setpgid (gpointer user_data) +{ + int fd = GPOINTER_TO_INT (user_data); + + /* Make sure the client end of the socket pair doesn't get closed + * when we exec xwayland. */ + int flags = fcntl (fd, F_GETFD); + if (flags != -1) + fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC); + + /* Put this process in a background process group, so that Ctrl-C + goes to mutter only */ + setpgid (0, 0); +} + +static void +xserver_died (GPid pid, + gint status, + gpointer user_data) +{ + if (!WIFEXITED (status)) + g_error ("X Wayland crashed; aborting"); + else + { + /* For now we simply abort if we see the server exit. + * + * In the future X will only be loaded lazily for legacy X support + * but for now it's a hard requirement. */ + g_error ("Spurious exit of X Wayland server"); + } +} + gboolean meta_xwayland_start (MetaWaylandCompositor *compositor) { @@ -190,6 +225,11 @@ meta_xwayland_start (MetaWaylandCompositor *compositor) char *lockfile = NULL; int sp[2]; pid_t pid; + char **env; + char *fd_string; + char *display_name; + char *args[11]; + GError *error; do { @@ -238,45 +278,39 @@ meta_xwayland_start (MetaWaylandCompositor *compositor) return 1; } - switch ((pid = fork())) + env = g_get_environ (); + fd_string = g_strdup_printf ("%d", sp[1]); + env = g_environ_setenv (env, "WAYLAND_SOCKET", fd_string, TRUE); + g_free (fd_string); + + display_name = g_strdup_printf (":%d", + compositor->xwayland_display_index); + + args[0] = XWAYLAND_PATH; + args[1] = display_name; + args[2] = "-wayland"; + args[3] = "-rootless"; + args[4] = "-retro"; + args[5] = "-noreset"; + args[6] = "-logfile"; + args[7] = g_build_filename (g_get_user_cache_dir (), "xwayland.log", NULL); + args[8] = "-nolisten"; + args[9] = "all"; + args[10] = NULL; + + error = NULL; + if (g_spawn_async (NULL, /* cwd */ + args, + env, + G_SPAWN_LEAVE_DESCRIPTORS_OPEN | + G_SPAWN_DO_NOT_REAP_CHILD | + G_SPAWN_STDOUT_TO_DEV_NULL | + G_SPAWN_STDERR_TO_DEV_NULL, + uncloexec_and_setpgid, + GINT_TO_POINTER (sp[1]), + &pid, + &error)) { - case 0: - { - char *fd_string; - char *display_name; - /* Make sure the client end of the socket pair doesn't get closed - * when we exec xwayland. */ - int flags = fcntl (sp[1], F_GETFD); - if (flags != -1) - fcntl (sp[1], F_SETFD, flags & ~FD_CLOEXEC); - - fd_string = g_strdup_printf ("%d", sp[1]); - setenv ("WAYLAND_SOCKET", fd_string, 1); - g_free (fd_string); - - display_name = g_strdup_printf (":%d", - compositor->xwayland_display_index); - - if (execl (XWAYLAND_PATH, - XWAYLAND_PATH, - display_name, - "-wayland", - "-rootless", - "-retro", - "-noreset", - /* FIXME: does it make sense to log to the filesystem by - * default? */ - "-logfile", "/tmp/xwayland.log", - "-nolisten", "all", - NULL) < 0) - { - char *msg = strerror (errno); - g_warning ("xwayland exec failed: %s", msg); - } - exit (-1); - return FALSE; - } - default: g_message ("forked X server, pid %d\n", pid); close (sp[1]); @@ -284,12 +318,15 @@ meta_xwayland_start (MetaWaylandCompositor *compositor) wl_client_create (compositor->wayland_display, sp[0]); compositor->xwayland_pid = pid; - break; - - case -1: - g_error ("Failed to fork for xwayland server"); - return FALSE; + g_child_watch_add (pid, xserver_died, NULL); } + else + { + g_error ("Failed to fork for xwayland server: %s", error->message); + } + + g_strfreev (env); + g_free (display_name); return TRUE; }