Merge lp:~stgraber/upstart/upstart-shell-paths into lp:upstart

Proposed by Stéphane Graber on 2012-12-17
Status: Merged
Merged at revision: 1408
Proposed branch: lp:~stgraber/upstart/upstart-shell-paths
Merge into: lp:upstart
Diff against target: 65 lines (+18/-3)
2 files modified
init/job_process.c (+10/-3)
init/tests/test_job_process.c (+8/-0)
To merge this branch: bzr merge lp:~stgraber/upstart/upstart-shell-paths
Reviewer Review Type Date Requested Status
James Hunt 2012-12-17 Approve on 2012-12-17
Review via email: mp+140175@code.launchpad.net

Description of the change

We recently noticed a bunch of upstart build failures caused by
test_job_process reporting that pWD=/ was added to the environment which
wasn't quite expected.

This was eventually tracked down to upstart calling the command through a
shell if any of the usual shell special characters are found (like ~ in this
case).

This branch does two things:
 - Changes job_process.c slightly to explicitly set ->script = TRUE when
   upstart decides that the command needs to be run under a shell.
 - Updates the tests to check for the post-run value of ->script and allow
   for PWD=/ in such case.

PWD will always be / if present as the shell itself is provided without any
environment and so can't inherit PWD.

To post a comment you must log in.
1409. By Stéphane Graber on 2012-12-17

Cleanup

James Hunt (jamesodhunt) wrote :

This is bug 1086474.

LGTM. Merged.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'init/job_process.c'
2--- init/job_process.c 2012-12-03 20:27:09 +0000
3+++ init/job_process.c 2012-12-17 11:39:20 +0000
4@@ -178,11 +178,18 @@
5 nih_assert (proc != NULL);
6 nih_assert (proc->command != NULL);
7
8+
9+ /* If the command string contains any shell-like characters,
10+ * automatically set script = TRUE since that's the best way to deal
11+ * with things like variables.
12+ */
13+ if (strpbrk (proc->command, SHELL_CHARS))
14+ proc->script = TRUE;
15+
16 /* We run the process using a shell if it says it wants to be run
17- * as such, or if it contains any shell-like characters; since that's
18- * the best way to deal with things like variables.
19+ * as such.
20 */
21- if ((proc->script) || strpbrk (proc->command, SHELL_CHARS)) {
22+ if (proc->script) {
23 char *nl, *p;
24
25 argc = 0;
26
27=== modified file 'init/tests/test_job_process.c'
28--- init/tests/test_job_process.c 2012-12-07 21:52:38 +0000
29+++ init/tests/test_job_process.c 2012-12-17 11:39:20 +0000
30@@ -680,6 +680,8 @@
31 output = fopen (filename, "r");
32 TEST_FILE_EQ (output, "BAR=BAZ\n");
33 TEST_FILE_EQ (output, "FOO=BAR\n");
34+ if (job->class->process[PROCESS_MAIN]->script)
35+ TEST_FILE_EQ (output, "PWD=/\n");
36 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
37 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
38 TEST_FILE_EQ (output, "UPSTART_NO_SESSIONS=1\n");
39@@ -734,6 +736,8 @@
40 output = fopen (filename, "r");
41 TEST_FILE_EQ (output, "BAR=BAZ\n");
42 TEST_FILE_EQ (output, "FOO=BAR\n");
43+ if (job->class->process[PROCESS_MAIN]->script)
44+ TEST_FILE_EQ (output, "PWD=/\n");
45 TEST_FILE_EQ (output, "UPSTART_INSTANCE=foo\n");
46 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
47 TEST_FILE_EQ (output, "UPSTART_NO_SESSIONS=1\n");
48@@ -790,6 +794,8 @@
49 TEST_FILE_EQ (output, "BAR=BAZ\n");
50 TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
51 TEST_FILE_EQ (output, "FOO=SMACK\n");
52+ if (job->class->process[PROCESS_PRE_STOP]->script)
53+ TEST_FILE_EQ (output, "PWD=/\n");
54 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
55 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
56 TEST_FILE_EQ (output, "UPSTART_NO_SESSIONS=1\n");
57@@ -846,6 +852,8 @@
58 TEST_FILE_EQ (output, "BAR=BAZ\n");
59 TEST_FILE_EQ (output, "CRACKLE=FIZZ\n");
60 TEST_FILE_EQ (output, "FOO=SMACK\n");
61+ if (job->class->process[PROCESS_POST_STOP]->script)
62+ TEST_FILE_EQ (output, "PWD=/\n");
63 TEST_FILE_EQ (output, "UPSTART_INSTANCE=\n");
64 TEST_FILE_EQ (output, "UPSTART_JOB=test\n");
65 TEST_FILE_EQ (output, "UPSTART_NO_SESSIONS=1\n");

Subscribers

People subscribed via source and target branches