[mutter/wayland] wayland: don't use fork() and SIGCHLD to spawn processes

[Giovanni Campagna <gcampagna src gnome org>]

commit 3803fd9511eb5341412507fa706f8098a37e4897
Author: Giovanni Campagna <gcampagn redhat com>
Date:   Mon Aug 12 09:46:07 2013 +0200

    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 276dba5..0326d8e 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 92f5d83..a859378 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 7c41c21..159499e 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 7ff30bf..56d3b71 100644
--- a/src/wayland/meta-xwayland.c
+++ b/src/wayland/meta-xwayland.c
@@ -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;
 }