Bug 47054

Looks fine to me but here's some bikeshedding:

+static void
+sigusr1_cb (int sig)
+{
+  mcd_service_stop (mcd);
 }

This doesn't seem to be the same indentation as the rest of the file?

+ g_object_unref (mcd);

use g_clear_object here if you want.

Simon is claiming that "using POSIX signals like that isn't safe unfortunately". Over to you, Simon.

(In reply to comment #1)
> Simon is claiming that "using POSIX signals like that isn't safe
> unfortunately". Over to you, Simon.

There's a relatively short list of things you can safely/portably do in a POSIX signal()/sigaction() handler. g_main_loop_quit() isn't one of them; neither is writing to a global variable (!) except with special precautions. A reasonable number of syscalls (but not all) are safe, though. The magic words to look for are "async-signal safe".

The signal(7) man page has the complete list of safe syscalls. In particular, write() and close() are safe.

The traditional way to handle a signal safely is to have a pipe-to-self, which you set up before entering the main loop. When you receive a signal, you write to the pipe (1 byte is enough). bus/main.c in dbus does this correctly.

All the signal handling should be #ifdef G_OS_UNIX, to avoid having to try to be portable at the same time as restricting yourself to async-signal-safe syscalls. signal()/sigaction() aren't portable beyond Unix anyway.

In GLib 2.32 we'll be able to use g_unix_signal_add() which is meant to do all this for you... but 2.32 isn't out yet, and I just looked at the implementation and found that it assumes single-byte writes to a volatile gchar[] are atomic, which I don't think is actually guaranteed.

I'd personally be inclined to use SIGINT (Ctrl+C) as the "graceful quit" signal, rather than SIGUSR1.

> + signal (SIGUSR1, sigusr1_cb);

The man page for signal() basically says "don't use this function": sigaction() is preferred (it has the same semantics on all platforms, whereas signal() varies).

> +static void
> +sigusr1_cb (int sig)
> +{
> + mcd_service_stop (mcd);
> }

Optional changes:

After you've changed this to be at the "read" end of the pipe-to-self, it might be better to have it raise the abort signal on the McdService (mcd_mission_abort) so that things get unreffed cleanly? That would hopefully mean we can eventually valgrind MC without having to use the special debug build and signal it over D-Bus.

From a valgrind point of view, it might also be good to do the actual main loop quit in a low-priority idle after the abort signal has happened, so that MC has a chance to clean up internal objects that were reffed by higher-priority idles. (The special debug build currently uses a 5 second timeout instead of an idle, but that seems as though it shouldn't be needed, ideally.)

Thanks for the review. I updated the code in the branch "exit3". It uses SIGINT, a pipe, mcd_mission_abort, and a low-priority idle.

(In reply to comment #3)
> Thanks for the review. I updated the code in the branch "exit3". It uses
> SIGINT, a pipe, mcd_mission_abort, and a low-priority idle.

Okay this looks fine to me. It would be nice if smcv could take a look too, but if not then go ahead.

> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <errno.h>
> +#include <string.h>
>  #include <glib.h>
> +#include <signal.h>

These additions should be #ifdef G_OS_UNIX. At least fcntl.h is non-portable.

> + retval = socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, quit_pipe);

Why a socket pair? All you need is a pipe - you don't need to write to the read end, read from the write end or use BSD Sockets API (which is what socketpair() gives you over pipe()).

SOCK_CLOEXEC is non-portable (Linux-only) but with a GLib 2.30 dependency, you can use g_unix_open_pipe (quit_pipe, FD_CLOEXEC, &error) which does The Right Thing on all platforms.

Telepathy coding style (space before parenthesis), please.

Fixed in branch "exit5":

(In reply to comment #5)
> > +#include <fcntl.h>
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +#include <errno.h>
> > +#include <string.h>
> >  #include <glib.h>
> > +#include <signal.h>
> 
> These additions should be #ifdef G_OS_UNIX. At least fcntl.h is non-portable.

Done. And #include <glib.h> is moved before the ifdef since it defines G_OS_UNIX. 

> > + retval = socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, quit_pipe);
> 
> Why a socket pair? All you need is a pipe - you don't need to write to the read
> end, read from the write end or use BSD Sockets API (which is what socketpair()
> gives you over pipe()).
> 
> SOCK_CLOEXEC is non-portable (Linux-only) but with a GLib 2.30 dependency, you
> can use g_unix_open_pipe (quit_pipe, FD_CLOEXEC, &error) which does The Right
> Thing on all platforms.

Done.

> Telepathy coding style (space before parenthesis), please.

Fixed.

Go for it! Thanks for your perseverance :-)

370e642..853fefb pushed on git master:

commit 853fefbb9663a5d6003cdefbc16c19c67d5bfcc7
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Fri Mar 16 15:32:51 2012 +0000

    Add mcd_service_stop() and stop the McdService when SIGINT is sent
    
    https://bugs.freedesktop.org/show_bug.cgi?id=47054

commit 72498e161d71b6751d825888a684f62c304a9434
Author: Alban Crequy <alban.crequy@collabora.co.uk>
Date:   Mon Mar 26 13:33:22 2012 +0100

    Update dependency on GLib 2.30
    
    GLib 2.30 is necessary for g_unix_open_pipe().
    
    https://bugs.freedesktop.org/show_bug.cgi?id=47054