[Merge] lp:~jamesodhunt/upstart/bug-530779 into lp:upstart

[Steve Langasek]

Review: Needs Fixing

+       TEST_FEATURE ("full double-fork daemon test where parent waits for ultimate child");

does not fit the expected structure of a test feature name.  Suggest instead:
        TEST_FEATURE ("with daemon where parent waits for ultimate child before exiting");

+       assert0 (prctl (PR_SET_CHILD_SUBREAPER, 1));

Upstart does not depend on PR_SET_CHILD_SUBREAPER being available on the running kernel - the test suite shouldn't either.  The test needs to be skipped if this facility is unavailable, or it needs to be rewritten to avoid subreaper entirely.

+               /* Low byte is signal */
+               TEST_EQ (siginfo.si_status, SIGTRAP);
+
+               /* Normally, the next byte would be the ptrace event,
+                * but upstart hasn't yet set PTRACE_O_TRACEEXEC, hence
+                * no ptrace event will be available.
+                */
+               TEST_EQ (((siginfo.si_status & ~0x7f)>>8), 0);

The second test here is redundant with the first; if siginfo.si_status == SIGTRAP, the high byte must be 0.  Either the second test should be dropped, or, if you want to keep it for clarity, the first test should be modified to check siginfo.si_status & 0x7f.

+               TIMED_BLOCK (5) {

I'm not thrilled about this TIMED_BLOCK() idea.  Why should we not just use inotify?  For that matter, why is this pidfile needed at all - what is this meant to guard against?  The test case already includes its own direct check of the pids with ptrace, so the only thing this pidfile does is make sure the daemon agrees with the test what the PIDs are.  That seems unnecessary to me.

+               /* Wait for child to send itself SIGSTOP denoting its
+                * final resting state.
+                */
+               got = FALSE;
+               TIMED_BLOCK (5) {
+
+                       memset (&siginfo, 0, sizeof (siginfo));
+                       ret = waitid (P_PID, final_pid, &siginfo, WSTOPPED | WNOWAIT | WUNTRACED);
+
+                       if (! ret && siginfo.si_code == CLD_STOPPED &&
+                                       siginfo.si_status == SIGSTOP) {
+                               got = TRUE;
+                               break;
+                       }
+               }

This section seems overly complicated.  The child process could just call pause() instead, and let the test runner kill the process when it's done.

+               TIMED_BLOCK (5) {
+                       nih_child_poll ();
+
+                       if (kill (final_pid, 0) == -1 && errno == ESRCH) {
+                               got = TRUE;
+                               break;
+                       }
+               }

Definitely no reason for this to be in a TIMED_BLOCK().  Or called at all.  We've already received CLD_EXITED above, so the kill() should immediately return ESRCH.  But again, this code all goes away if we just kill SIGKILL the final_pid, instead of including these checks that are unrelated to the purpose of the test case.

 static void
 selfpipe_setup (void)
 {
-    static struct sigaction  act;
-    int                      read_flags;
-    int                      write_flags;
[...]

This mixes formatting changes with functional changes, making it much harder to follow the history.  Please split the format changes into a separate commit and rebase the functional changes on top.
-- 
https://code.launchpad.net/~jamesodhunt/upstart/bug-530779/+merge/197080
Your team Upstart Reviewers is subscribed to branch lp:upstart.