Created attachment 218434 [details] [review]
Add g_spawn_check_exit_status()

Created attachment 218435 [details]
sample gnome-session patch

Comment on attachment 218434 [details] [review]
Add g_spawn_check_exit_status()

>+ * If for some reason you do need to make your program behave
>+ * differently if a child (for example) exits with code 1 versus when
>+ * it exits with code 2, you will need to use platform-specific APIs
>+ * such as Unix <literal>WIFEXITED()</literal> and
>+ * <literal>WEXITSTATUS()</literal>.

Hm... I thought I'd made this comment in the GSubprocess bug, but maybe it was just on IRC. Anyway, it would be nice to have a G_PROCESS_EXIT_ERROR GError domain, and then in the exited-with-non-0-status case, do

	  g_set_error (error, G_PROCESS_EXIT_ERROR, WEXITSTATUS (exit_status),
		       _("Child process exited with code %ld"),
		       (long) WEXITSTATUS (exit_status));

which lets you extract the exit status from the GError if you want it.

(In reply to comment #3)
> (From update of attachment 218434 [details] [review])
> >+ * If for some reason you do need to make your program behave
> >+ * differently if a child (for example) exits with code 1 versus when
> >+ * it exits with code 2, you will need to use platform-specific APIs
> >+ * such as Unix <literal>WIFEXITED()</literal> and
> >+ * <literal>WEXITSTATUS()</literal>.
> 
> Hm... I thought I'd made this comment in the GSubprocess bug, but maybe it was
> just on IRC. Anyway, it would be nice to have a G_PROCESS_EXIT_ERROR GError
> domain, and then in the exited-with-non-0-status case, do
> 
>       g_set_error (error, G_PROCESS_EXIT_ERROR, WEXITSTATUS (exit_status),
>                _("Child process exited with code %ld"),
>                (long) WEXITSTATUS (exit_status));
> 
> which lets you extract the exit status from the GError if you want it.

But what do I do if it was killed by a signal?

just return G_SPAWN_ERROR_FAILED like you do now; it's MUCH rarer that you'd actually care *which* signal killed the process (for any reason other than putting it into the error message).

(In reply to comment #5)
> just return G_SPAWN_ERROR_FAILED like you do now; it's MUCH rarer that you'd
> actually care *which* signal killed the process (for any reason other than
> putting it into the error message).

Can you point me to code that cares about the exit status and doesn't care about the signal?  gnome-session does this (it specifically checks for exit code 1 and special cases it), but honestly I think what it's doing is total crack.

My guess is that most code that is looking at the exit code is trying to do custom error messages.  And if you're doing custom error messages, you probably want to know which signal too.  Of course, I'd also guess that most code that presently has custom error messages would be totally fine with the ones we have now in g_spawn_check_exit_status(); at least I feel that's the case for the gnome-session patch (which note is a different code section than the total crack one above).

(In reply to comment #6)
> Can you point me to code that cares about the exit status and doesn't care
> about the signal?

They're completely different sorts of things; programs intentionally exit with specific non-0 status codes to communicate information to their caller. Exiting on a signal is almost always unintentional on the part of the exiting program, and so carries very little information.

grepping through a jhbuild, I guess I should have said "*command line* programs" above:

  - metacity invokes zenity and looks at the exit status to determine,
    eg, what button was clicked. (Zenity can do lots of stuff with exit
    status, and in particular, it differentiates "user clicked cancel"
    from internal errors like "couldn't find .ui file".)

  - NM uses the exit codes of pppd and dnsmasq to provide specific error
    messages explaining what failed. ("man pppd" and search for "EXIT
    STATUS")

  - The evolution spamassassin and bogofilter plugins use the exit status
    of the program they call to determine whether the message is spam or not
    (and, as in the zenity case, to differentiate that from "something went
    wrong")

So, not super common I guess, but it's there.

(There are also lots of programs like dbus-launch and pk-spawn that "re-throw" the exit status of their child.)

(In reply to comment #7)
> (In reply to comment #6)
> > Can you point me to code that cares about the exit status and doesn't care
> > about the signal?
> 
> They're completely different sorts of things; programs intentionally exit with
> specific non-0 status codes to communicate information to their caller. Exiting
> on a signal is almost always unintentional on the part of the exiting program,
> and so carries very little information.

Ok...so code would look like:

  GError *spawn_error = NULL;

  /* g_spawn_sync(...) */

  if (!g_spawn_check_exit_status (exit_status, &spawn_error))
    {
      if (spawn_error->domain == G_SPAWN_EXIT_ERROR)
        {
          if (spawn_error->code == 0)
            /* Ok was pressed */
          else if (spawn_error->code == 1)
            /* Cancel was pressed */
          else
            {
              g_propagate_error (error, spawn_error);
              goto out;
            }
        }
      else
        {
          g_propagate_error (error, spawn_error);
          goto out;
        }
    }

But the thing is, it's basically the same amount of code to do #include <sys/wait.h>, and:

    if (WIFEXITED (exit_status))
      {
        if (WEXITSTATUS (exit_status) == 1)
          /* OK */
        else if (WEXITSTATUS (exit_status) == 2)
          /* Cancel */
        else
          {
            g_spawn_check_exit_status (exit_status, error);
            goto out;
          }
      }
    else
      {
        g_spawn_check_exit_status (exit_status, error);
        goto out;
      }
    }

What I'm not sure of though is whether you can use exit codes on Windows in the same way you can on Unix.  From my reading of http://msdn.microsoft.com/en-us/library/windows/desktop/ms682658%28v=vs.85%29.aspx you probably can...but I don't think any of the listed projects care about portability to Windows.

>  else
>    g_propagate_error (error, spawn_error);

well, I think these would mostly be in situations where the g_spawn* caller was also the final consumer of the GError. Eg, the metacity force-quit handler doesn't care that zenity segfaulted, it just wants to know whether or not it should continue with the force-quitting.

But yes, it's not any less code, just more glib-like (and bindable!)

Created attachment 218472 [details] [review]
Add g_spawn_check_exit_status()

Now using G_SPAWN_EXIT_ERROR

Review of attachment 218472 [details] [review]:

I like it. Feel free to commit after fixing the cosmetic issues.

::: gio/glib-compile-resources.c
@@ +330,3 @@
                 }
+	      
+	      /* Ugly...we shoud probably just keep stderr to our stderr */

that comment is hard to parse. 'keep stderr to our stderr' ?!

::: glib/gspawn.c
@@ +241,3 @@
+ *
+ * If @exit_status is non-%NULL, the platform-specific exit status of
+ * the child is stored there; see the doucumentation of

typo: doucumentation

@@ +836,3 @@
+ * on @exit_status directly.  Do not attempt to scan or parse the
+ * error message string; it may be translated and/or change in future
+ * versions of GLib.

Putting the example code from the bug in here would be nice.

@@ +837,3 @@
+ * error message string; it may be translated and/or change in future
+ * versions of GLib.
+ *

You're missing the documentation for the return value here.

(In reply to comment #11)

> Putting the example code from the bug in here would be nice.

I considered it but gspawn.c doesn't have any example code at all right now, and I'd be loath to add some just for people living in a state of sin^W^W^W interpreting error codes.  Plus the doc comment is already too long.

If anything I want to add more examples of how to use GSubprocess =)  The gio/tests/gsubprocess.c already uses G_SPAWN_EXIT_ERROR.

Attachment 218472 [details] pushed as f7abd3c - Add g_spawn_check_exit_status()

unfortunately, that break windows compilation, (reopening to avoid extra bugs)

(In reply to comment #14)
> unfortunately, that break windows compilation, (reopening to avoid extra bugs)

Build logs?

Created attachment 218576 [details] [review]
win32: fix build g_spawn_check_exit_status() with mingw

With mingw, only gspawn-win32.c is compiled, but it is missing some
new symbols

(In reply to comment #15)
> (In reply to comment #14)
> > unfortunately, that break windows compilation, (reopening to avoid extra bugs)
> 
> Build logs?

  CCLD   libglib-2.0.la
Creating library file: .libs/libglib-2.0.dll.a
Cannot export g_spawn_check_exit_status: symbol not defined
Cannot export g_spawn_exit_error_quark: symbol not defined
collect2: error: ld returned 1 exit status

Review of attachment 218576 [details] [review]:

Oh...ugh.  Can you make the gspawn.c only apply to Unix then too?  Delete the #ifdef G_OS_UNIX?  Duplicating the error quark is mildly lame, but oh well.

Review of attachment 218576 [details] [review]:

gspawn.c is compiled for anything but mingw apparently, so we may want to keep the #ifdef G_OS_UNIX, no?

AC_MSG_CHECKING(for gspawn implementation)
case "$host" in
  *-*-mingw*)
    GSPAWN=gspawn-win32.lo
    ;;
  *)
    GSPAWN=gspawn.lo
    ;;    
esac
AC_MSG_RESULT($GSPAWN)
AC_SUBST(GSPAWN)

For the quark, I thought about putting it elsewhere, but I had no good idea :)

(In reply to comment #19)
> Review of attachment 218576 [details] [review]:
> 
> gspawn.c is compiled for anything but mingw apparently, so we may want to keep
> the #ifdef G_OS_UNIX, no?
> 
> AC_MSG_CHECKING(for gspawn implementation)
> case "$host" in
>   *-*-mingw*)
>     GSPAWN=gspawn-win32.lo

I don't understand this...because gspawn.c will not compile on Windows in any configuration, right?  I mean, it uses fork() etc.  So we must not be supporting non-mingw win32 builds?

we don't support non-mingw builds via ./configure. There's a separate MSVC build system in build/win32 that you're allowed to not think about how it works.

(In reply to comment #21)
> we don't support non-mingw builds via ./configure. There's a separate MSVC
> build system in build/win32 that you're allowed to not think about how it
> works.

WALL OF UUIDS AND XML! MY EYES!!!

So...yeah I think this supports my assertion that we should remove the win32 code I thought would work from gspawn.c.

Created attachment 218584 [details] [review]
win32: fix build g_spawn_check_exit_status() with mingw

With mingw, only gspawn-win32.c is compiled, but it is missing some
new symbols.

Review of attachment 218584 [details] [review]:

Go go gadget commit!

(In reply to comment #22)
> WALL OF UUIDS AND XML! MY EYES!!!

also known as the systemd build system

(ok, I'll shut up, I promise)

Attachment 218584 [details] pushed as d9af425 - win32: fix build g_spawn_check_exit_status() with mingw

(In reply to comment #5)
> just return G_SPAWN_ERROR_FAILED like you do now; it's MUCH rarer that you'd
> actually care *which* signal killed the process (for any reason other than
> putting it into the error message).

I don't know if anything *actually* uses that, but this from the bash docs:

   The return status (*note Exit Status::) of a simple command is its
exit status as provided by the POSIX 1003.1 `waitpid' function, or
128+N if the command was terminated by signal N.

so I guess someone might care. (E.g. in g-t I emulate this behaviour.)