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.
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
