[gnome-session] gsm-process-helper: Give useful error data

[Colin Walters <walters src gnome org>]

commit c0840a766a02ba3f920bc810c938e6c6d13e897c
Author: Colin Walters <walters verbum org>
Date:   Thu Mar 3 18:55:16 2011 -0500

    gsm-process-helper: Give useful error data
    
    In order for engineers to be able to extract why a tool
    like gnome-session-check-accelerated failed, we should print
    something to ~/.xsession-errors.
    
    gsm_process_helper() only returned the exit code, unless the process
    was killed by a signal, in which case it returned -1.  However
    the only consumer of the code never checked the exit code, just
    success.
    
    So fix this by having gsm_process_helper return a normal
    gboolean/GError pair.  As part of this, clean up the code so that it
    also handles the WIFSIGNALED and WIFSTOPPED cases (See "man 2 waitpid").
    
    Also, the exit_child_simple() function was lame; we don't
    need to call g_spawn_close_pid() on Unix, so don't do it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=643880

 gnome-session/gsm-process-helper.c  |   66 ++++++++++++++++++++--------------
 gnome-session/gsm-process-helper.h  |    5 ++-
 gnome-session/gsm-session-fill.c    |    7 +++-
 gnome-session/test-process-helper.c |   14 ++++----
 4 files changed, 55 insertions(+), 37 deletions(-)
---
diff --git a/gnome-session/gsm-process-helper.c b/gnome-session/gsm-process-helper.c
index c36fe2a..0a1fb97 100644
--- a/gnome-session/gsm-process-helper.c
+++ b/gnome-session/gsm-process-helper.c
@@ -31,7 +31,7 @@
 typedef struct {
         const char *command_line;
         GPid        pid;
-        gboolean    proper_exit;
+        gboolean    timed_out;
         int         status;
         GMainLoop  *loop;
         guint       child_id;
@@ -39,21 +39,13 @@ typedef struct {
 } GsmProcessHelper;
 
 static void
-on_child_exited_simple (GPid     pid,
-                        gint     status,
-                        gpointer data)
-{
-        g_spawn_close_pid (pid);
-}
-
-static void
 on_child_exited (GPid     pid,
                  gint     status,
                  gpointer data)
 {
         GsmProcessHelper *helper = data;
 
-        helper->proper_exit = TRUE;
+        helper->timed_out = FALSE;
         helper->status = status;
 
         g_spawn_close_pid (pid);
@@ -66,26 +58,24 @@ on_child_timeout (gpointer data)
         GsmProcessHelper *helper = data;
 
         kill (helper->pid, SIGTERM);
-        g_warning ("Had to kill '%s' helper", helper->command_line);
-
-        helper->proper_exit = FALSE;
+        helper->timed_out = TRUE;
         g_main_loop_quit (helper->loop);
 
         return FALSE;
 }
 
-int
+gboolean
 gsm_process_helper (const char   *command_line,
-                    unsigned int  timeout)
+                    unsigned int  timeout,
+                    GError      **error)
 {
         GsmProcessHelper *helper;
         gchar **argv = NULL;
         GPid pid;
         gboolean ret;
-        int exit_status = -1;
 
-        if (!g_shell_parse_argv (command_line, NULL, &argv, NULL))
-                return -1;
+        if (!g_shell_parse_argv (command_line, NULL, &argv, error))
+                return FALSE;
 
         ret = g_spawn_async (NULL,
                              argv,
@@ -94,18 +84,20 @@ gsm_process_helper (const char   *command_line,
                              NULL,
                              NULL,
                              &pid,
-                             NULL);
+                             error);
 
         g_strfreev (argv);
 
         if (!ret)
-                return -1;
+                return FALSE;
+
+        ret = FALSE;
 
         helper = g_slice_new0 (GsmProcessHelper);
 
         helper->command_line = command_line;
         helper->pid = pid;
-        helper->proper_exit = FALSE;
+        helper->timed_out = FALSE;
         helper->status = -1;
 
         helper->loop = g_main_loop_new (NULL, FALSE);
@@ -114,11 +106,31 @@ gsm_process_helper (const char   *command_line,
 
         g_main_loop_run (helper->loop);
 
-        if (helper->proper_exit && WIFEXITED (helper->status))
-                exit_status = WEXITSTATUS (helper->status);
-        else if (!helper->proper_exit) {
-                /* we'll need to call g_spawn_close_pid when the helper exits */
-                g_child_watch_add (pid, on_child_exited_simple, NULL);
+        if (helper->timed_out) {
+                g_set_error_literal (error,
+                                     G_IO_CHANNEL_ERROR,
+                                     G_IO_CHANNEL_ERROR_FAILED,
+                                     "Timed out");
+        } else {
+                if (WIFEXITED (helper->status)) {
+                        if (WEXITSTATUS (helper->status) == 0)
+                                ret = TRUE;
+                        else
+                                g_set_error (error,
+                                             G_IO_CHANNEL_ERROR,
+                                             G_IO_CHANNEL_ERROR_FAILED,
+                                             "Exited with code %d", WEXITSTATUS (helper->status));
+                } else if (WIFSIGNALED (helper->status)) {
+                        g_set_error (error,
+                                     G_IO_CHANNEL_ERROR,
+                                     G_IO_CHANNEL_ERROR_FAILED,
+                                     "Killed by signal %d", WTERMSIG (helper->status));
+                } else if (WIFSTOPPED (helper->status)) {
+                        g_set_error (error,
+                                     G_IO_CHANNEL_ERROR,
+                                     G_IO_CHANNEL_ERROR_FAILED,
+                                     "Stopped by signal %d", WSTOPSIG (helper->status));
+                }
         }
 
         if (helper->loop) {
@@ -138,5 +150,5 @@ gsm_process_helper (const char   *command_line,
 
         g_slice_free (GsmProcessHelper, helper);
 
-        return exit_status;
+        return ret;
 }
diff --git a/gnome-session/gsm-process-helper.h b/gnome-session/gsm-process-helper.h
index f7aa18c..f02ca37 100644
--- a/gnome-session/gsm-process-helper.h
+++ b/gnome-session/gsm-process-helper.h
@@ -25,8 +25,9 @@
 
 G_BEGIN_DECLS
 
-int  gsm_process_helper (const char   *command_line,
-                         unsigned int  timeout);
+int  gsm_process_helper (const char    *command_line,
+                         unsigned int   timeout,
+                         GError       **error);
 
 G_END_DECLS
 
diff --git a/gnome-session/gsm-session-fill.c b/gnome-session/gsm-session-fill.c
index 139e9be..877eec2 100644
--- a/gnome-session/gsm-session-fill.c
+++ b/gnome-session/gsm-session-fill.c
@@ -367,6 +367,7 @@ get_session_keyfile (const char *session,
         GKeyFile *keyfile;
         gboolean  session_runnable;
         char     *value;
+        GError *error = NULL;
 
         *actual_session = NULL;
 
@@ -384,7 +385,11 @@ get_session_keyfile (const char *session,
                                        NULL);
         if (!IS_STRING_EMPTY (value)) {
                 g_debug ("fill: *** Launching helper '%s' to know if session is runnable", value);
-                session_runnable = (gsm_process_helper (value, GSM_RUNNABLE_HELPER_TIMEOUT) == 0);
+                session_runnable = gsm_process_helper (value, GSM_RUNNABLE_HELPER_TIMEOUT, &error);
+                if (!session_runnable) {
+                        g_warning ("Session '%s' runnable check failed:", error->message);
+                        g_clear_error (&error);
+                }
         }
         g_free (value);
 
diff --git a/gnome-session/test-process-helper.c b/gnome-session/test-process-helper.c
index 50f79ad..7fe9216 100644
--- a/gnome-session/test-process-helper.c
+++ b/gnome-session/test-process-helper.c
@@ -29,9 +29,9 @@ int
 main (int   argc,
       char *argv[])
 {
-        int   ret;
         char *command_line = "xeyes";
         int   timeout = 500;
+        GError *error = NULL;
 
         if (argc > 3) {
                 g_printerr ("Too many arguments.\n");
@@ -47,12 +47,12 @@ main (int   argc,
                         timeout = i;
         }
 
-        ret = gsm_process_helper (command_line, timeout);
-
-        if (ret < 0)
-                g_print ("Command did not succeed (or takes too much time).\n");
-        else
-                g_print ("Command returned %d.\n", ret);
+        if (!gsm_process_helper (command_line, timeout, &error)) {
+                g_warning ("%s", error->message);
+                g_clear_error (&error);
+        } else {
+                g_print ("Command exited successfully.\n");
+        }
 
         return 0;
 }