Merge lp:~vanvugt/mir/fix-1237710 into lp:mir

[Daniel van Vugt]

Fix failing acceptance-test:
ServerShutdown/OnSignal.removes_endpoint_on_signal (LP: #1237710)

Avoid fatal_signal_cleanup getting caught in a loop restoring itself and
then re-entering itself. This could happen in some permutations of
acceptance-tests where run_mir is entered with fatal_signal_cleanup already
set up from previous tests' server instances.

[lp:~vanvugt/mir/fix-1237710]

1140. By Daniel van Vugt on 2013-10-16

Also remove unused typedef

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1139
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/177/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2385
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2270
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/174
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/174/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/177/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1140
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/179/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2387
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/176
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/176/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/179/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

*Needs discussion*

I'm concerned about the implicit assumption that the previous signal handlers are necessarily the default. See an alternative approach at: https://code.launchpad.net/~alan-griffiths/mir/fix-1237710/+merge/191412

review: Needs Information

[Kevin DuBois (kdub)]

I like restoring the handlers (as in lp:~alan-griffiths/mir/fix-1237710), but I'd be okay with this too

review: Approve

[Kevin DuBois (kdub)]

I'll switch to abstain (would prefer lp:~alan-griffiths/mir/fix-1237710)

review: Abstain

[Alexandros Frantzis (afrantzis)]

7 + signal(sig, SIG_DFL);

Since we are a library, I don't think we should be resetting signals to default. Other linked code may also need to perform some cleanup.

[Robert Carr (robertcarr)]

review: Abstain

[Daniel van Vugt (vanvugt)]

The same concern occurred to me last night.

Unmerged revisions

1140. By Daniel van Vugt on 2013-10-16

Also remove unused typedef

1139. By Daniel van Vugt on 2013-10-16

Avoid fatal_signal_cleanup getting caught in a loop restoring itself and
then re-entering itself. This could happen in some permutations of
acceptance-tests where run_mir is entered with fatal_signal_cleanup already
set up from previous server instance tests. (LP: #1237710)

=== modified file 'src/server/run_mir.cpp'
--- src/server/run_mir.cpp	2013-10-14 06:33:42 +0000
+++ src/server/run_mir.cpp	2013-10-16 09:40:57 +0000
@@ -39,15 +39,11 @@
     }
 }
 
-extern "C" { typedef void (*sig_handler)(int); }
-
-volatile sig_handler old_handler[SIGUNUSED]  = { nullptr };
-
 extern "C" void fatal_signal_cleanup(int sig)
 {
     delete_endpoint();
 
-    signal(sig, old_handler[sig]);
+    signal(sig, SIG_DFL);
     raise(sig);
 }
 }
@@ -72,7 +68,7 @@
     weak_connector = config.the_connector();
 
     for (auto sig : { SIGQUIT, SIGABRT, SIGFPE, SIGSEGV, SIGBUS })
-        old_handler[sig] = signal(sig, fatal_signal_cleanup);
+        signal(sig, fatal_signal_cleanup);
 
     std::atexit(&delete_endpoint);