brl/mutter
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

wayland: don't use fork() and SIGCHLD to spawn processes

Browse Source 
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
This commit is contained in:
Giovanni Campagna 2013-08-12 09:46:07 +02:00
parent 152d896f75
commit 3803fd9511
4 changed files with 80 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/core/main.c
View File

@ -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"))

2
src/wayland/meta-wayland-private.h
View File

@ -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,

24
src/wayland/meta-wayland.c
View File

@ -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");
}
}
}

123
src/wayland/meta-xwayland.c
View File

@ -28,6 +28,8 @@
#include <errno.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/wait.h>
#include <stdlib.h>
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;
}