Merge lp:~jamesodhunt/upstart/bug-1269731 into lp:upstart

Proposed by James Hunt on 2014-01-17
Status: Merged
Merged at revision: 1591
Proposed branch: lp:~jamesodhunt/upstart/bug-1269731
Merge into: lp:upstart
Diff against target: 295 lines (+149/-18)
7 files modified
ChangeLog (+23/-0)
init/conf.c (+16/-2)
init/job_class.c (+3/-0)
init/tests/test_conf.c (+77/-12)
scripts/pyupstart.py (+3/-2)
scripts/tests/test_pyupstart_session_init.py (+15/-2)
scripts/tests/test_pyupstart_system_init.py (+12/-0)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1269731
Reviewer Review Type Date Requested Status
Colin Watson (community) 2014-01-17 Approve on 2014-01-21
Review via email: mp+202083@code.launchpad.net
To post a comment you must log in.
lp:~jamesodhunt/upstart/bug-1269731 updated on 2014-01-17
1592. By James Hunt on 2014-01-17

* scripts/pyupstart.py:
  - JobInstance:destroy(): Whitespace.
  - SessionInit::reexec(): Add a log message.
* scripts/tests/test_pyupstart_session_init.py:
  - TestSessionUpstart: Add pid to ps output.
  - TestSessionInitReExec::test_session_init_reexec():
    - Create an invalid job to ensure re-exec can handle it.
    - Use 'with' for handling re-exec exception.
* scripts/tests/test_pyupstart_system_init.py:
  - TestSystemInitReExec::test_pid1_reexec():
    - Create an invalid job to ensure re-exec can handle it.

James Hunt (jamesodhunt) wrote :

Note that the Python scenario tests will fail unless /sbin/init on the test system actually has the fix for bug 1269731. This will be much easier to test once lp:~jamesodhunt/upstart/fix-python-tests gets merged into lp:upstart since that allows the tests to run against different Upstart binaries.

Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2014-01-15 11:51:18 +0000
3+++ ChangeLog 2014-01-17 15:40:28 +0000
4@@ -1,3 +1,26 @@
5+2014-01-17 James Hunt <james.hunt@ubuntu.com>
6+
7+ * init/conf.c:
8+ - conf_file_serialise(): Handle ConfFile's without an
9+ associated JobClass, resulting from an unparseable job configuration
10+ file (LP: #1269731).
11+ - conf_file_deserialise(): Comments.
12+ * init/job_class.c: job_class_deserialise_all(): Comments.
13+ * init/tests/test_conf.c: test_source_reload_file(): Add new tests:
14+ - "Invalid .conf file does not stop ConfFile being serialised".
15+ - "ConfFile with no JobClass can be deserialised".
16+ * scripts/pyupstart.py:
17+ - JobInstance:destroy(): Whitespace.
18+ - SessionInit::reexec(): Add a log message.
19+ * scripts/tests/test_pyupstart_session_init.py:
20+ - TestSessionUpstart: Add pid to ps output.
21+ - TestSessionInitReExec::test_session_init_reexec():
22+ - Create an invalid job to ensure re-exec can handle it.
23+ - Use 'with' for handling re-exec exception.
24+ * scripts/tests/test_pyupstart_system_init.py:
25+ - TestSystemInitReExec::test_pid1_reexec():
26+ - Create an invalid job to ensure re-exec can handle it.
27+
28 2014-01-15 James Hunt <james.hunt@ubuntu.com>
29
30 * util/telinit.c: upstart_open(): Connect using private socket
31
32=== modified file 'init/conf.c'
33--- init/conf.c 2013-11-16 12:42:57 +0000
34+++ init/conf.c 2014-01-17 15:40:28 +0000
35@@ -1636,6 +1636,19 @@
36 if (! state_set_json_int_var_from_obj (json, file, flag))
37 goto error;
38
39+ if (! file->job) {
40+ /* File exists on disk but contains invalid
41+ * (unparseable) syntax, and hence no associated JobClass.
42+ * Thus, simply encode the ConfFile without a class.
43+ *
44+ * Deserialisation is handled automatically since
45+ * JobClasses are deserialised by directly iterating
46+ * through all JobClass'es found in the JSON. Here,
47+ * there simply won't be a JobClass to deserialise.
48+ */
49+ goto out;
50+ }
51+
52 /*
53 * Ignore the "best" JobClass associated with this ConfFile
54 * (file->job) since it won't be serialised.
55@@ -1681,6 +1694,7 @@
56
57 json_object_object_add (json, "job_class", json_job_class);
58
59+out:
60 return json;
61
62 error:
63@@ -1717,8 +1731,8 @@
64 goto error;
65
66 /* Note that the associated JobClass is not handled at this
67- * stage: it can't be the JobClasses haven't been deserialised
68- * yet. As such, the ConfFile->JobClass link is dealt with by
69+ * stage: it can't be since the JobClasses haven't been deserialised
70+ * yet. As such, the ConfFile->JobClass link is dealt with in
71 * job_class_deserialise_all().
72 */
73 file->job = NULL;
74
75=== modified file 'init/job_class.c'
76--- init/job_class.c 2013-11-04 09:34:51 +0000
77+++ init/job_class.c 2014-01-17 15:40:28 +0000
78@@ -2429,6 +2429,9 @@
79 if (! state_check_json_type (json_class, object))
80 goto error;
81
82+ /* Responsible for associating a JobClass with its
83+ * parent ConfFile.
84+ */
85 class = job_class_deserialise (json_class);
86
87 /* Either memory is low or -- more likely -- a JobClass
88
89=== modified file 'init/tests/test_conf.c'
90--- init/tests/test_conf.c 2013-06-25 10:13:12 +0000
91+++ init/tests/test_conf.c 2014-01-17 15:40:28 +0000
92@@ -3724,14 +3724,15 @@
93 void
94 test_source_reload_file (void)
95 {
96- ConfSource *source;
97- ConfFile *file, *old_file;
98- FILE *f;
99- int ret, fd, nfds;
100- char dirname[PATH_MAX];
101- char tmpname[PATH_MAX], filename[PATH_MAX];
102- fd_set readfds, writefds, exceptfds;
103- NihError *err;
104+ ConfSource *source;
105+ ConfFile *file, *old_file;
106+ FILE *f;
107+ int ret, fd, nfds;
108+ char dirname[PATH_MAX];
109+ char tmpname[PATH_MAX], filename[PATH_MAX];
110+ fd_set readfds, writefds, exceptfds;
111+ NihError *err;
112+ json_object *json;
113
114 TEST_FUNCTION_FEATURE ("conf_source_reload",
115 "with configuration file");
116@@ -4500,12 +4501,76 @@
117 strcat (filename, "/bar.conf");
118 unlink (filename);
119
120+ /* Re-enable inotify */
121+ assert0 (unsetenv ("INOTIFY_DISABLE"));
122+
123+ TEST_FEATURE ("Invalid .conf file does not stop ConfFile being serialised");
124+ conf_init ();
125+ TEST_LIST_EMPTY (conf_sources);
126+
127+ f = fopen (filename, "w");
128+ /* Create an invalid job by adding an invalid stanza */
129+ fprintf (f, "invalid\n");
130+ fclose (f);
131+
132+ source = conf_source_new (NULL, filename, CONF_FILE);
133+ TEST_NE_P (source, NULL);
134+ TEST_LIST_NOT_EMPTY (conf_sources);
135+ TEST_HASH_EMPTY (source->files);
136+
137+ file = conf_file_new (source, "/path/to/file");
138+ TEST_NE_P (file, NULL);
139+ TEST_HASH_NOT_EMPTY (source->files);
140+
141+ /* Initially, a ConfFile has no associated JobClass */
142+ TEST_EQ_P (file->job, NULL);
143+
144+ /* Normally, this would create a JobClass and associate it with
145+ * its parent ConfFile, but that doesn't happen when the on-disk
146+ * job configuration file is invalid.
147+ */
148+ ret = conf_source_reload (source);
149+
150+ /* Although the on-disk file is invalid, there is no error here
151+ * since it's already been handled by conf_reload_path() (which
152+ * displays an error message with details of how the job
153+ * configuration file is invalid).
154+ */
155+ TEST_EQ (ret, 0);
156+
157+ /* We know the job was invalid * by the fact that the ConfFile
158+ * still has no associated JobClass.
159+ */
160+ TEST_EQ (file->job, NULL);
161+
162+ /* See if we can serialise the ConfFile without an associated
163+ * JobClass.
164+ */
165+ json = conf_file_serialise (file);
166+ TEST_NE_P (json, NULL);
167+
168+ /* Test there is no JobClass in the JSON */
169+ TEST_EQ_P (json_object_object_get (json, "job_class"), NULL);
170+
171+ TEST_FEATURE ("ConfFile with no JobClass can be deserialised");
172+
173+ nih_free (source);
174+
175+ source = conf_source_new (NULL, filename, CONF_FILE);
176+ TEST_NE_P (source, NULL);
177+ TEST_LIST_NOT_EMPTY (conf_sources);
178+ TEST_HASH_EMPTY (source->files);
179+
180+ file = conf_file_deserialise (source, json);
181+ TEST_NE_P (file, NULL);
182+
183+ nih_free (source);
184+
185+ json_object_put (json);
186+
187+ unlink (filename);
188 rmdir (dirname);
189-
190 nih_log_set_priority (NIH_LOG_MESSAGE);
191-
192- /* Re-enable inotify */
193- assert0 (unsetenv ("INOTIFY_DISABLE"));
194 }
195
196
197
198=== modified file 'scripts/pyupstart.py'
199--- scripts/pyupstart.py 2013-09-12 10:56:22 +0000
200+++ scripts/pyupstart.py 2014-01-17 15:40:28 +0000
201@@ -925,8 +925,8 @@
202 """
203 Stop the instance and cleanup.
204
205- Note: If the instance specified retain when created, this will
206- be a NOP.
207+ Note: If the instance specified retain when created, this will
208+ be a NOP.
209 """
210 if not self.job.retain:
211 self.stop()
212@@ -1119,4 +1119,5 @@
213 if not self.proxy:
214 raise UpstartException('Not yet connected')
215
216+ self.logger.debug("Restarting Session Init")
217 self.proxy.Restart()
218
219=== modified file 'scripts/tests/test_pyupstart_session_init.py'
220--- scripts/tests/test_pyupstart_session_init.py 2013-11-03 00:28:07 +0000
221+++ scripts/tests/test_pyupstart_session_init.py 2014-01-17 15:40:28 +0000
222@@ -51,7 +51,7 @@
223 FILE_BRIDGE_CONF = 'upstart-file-bridge.conf'
224 REEXEC_CONF = 're-exec.conf'
225
226- PSCMD_FMT = 'ps --no-headers -p %d -o comm,args'
227+ PSCMD_FMT = 'ps --no-headers -p %d -o pid,comm,args'
228
229 def setUp(self):
230
231@@ -343,6 +343,16 @@
232 version = self.upstart.version()
233 self.assertTrue(version)
234
235+ # Create an invalid job to ensure this causes no problems for
236+ # the re-exec. Note that we cannot use job_create() since
237+ # that validates the syntax of the .conf file).
238+ #
239+ # We create this file before any other to allow time for Upstart
240+ # to _attempt to parse it_ by the time the re-exec is initiated.
241+ invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
242+ with open(invalid_conf, 'w', encoding='utf-8') as fh:
243+ print("invalid", file=fh)
244+
245 # create a job and start it, marking it such that the .conf file
246 # will be retained when object becomes unusable (after re-exec).
247 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
248@@ -360,7 +370,8 @@
249 # Trigger re-exec and catch the D-Bus exception resulting
250 # from disconnection from Session Init when it severs client
251 # connections.
252- self.assertRaises(dbus.exceptions.DBusException, self.upstart.reexec)
253+ with self.assertRaises(dbus.exceptions.DBusException):
254+ self.upstart.reexec()
255
256 os.kill(self.upstart.pid, 0)
257
258@@ -411,6 +422,8 @@
259 with self.assertRaises(ProcessLookupError):
260 os.kill(pid, 0)
261
262+ os.remove(invalid_conf)
263+
264 self.stop_session_init()
265
266 def test_session_init_reexec_when_pid1_does(self):
267
268=== modified file 'scripts/tests/test_pyupstart_system_init.py'
269--- scripts/tests/test_pyupstart_system_init.py 2013-09-12 10:56:22 +0000
270+++ scripts/tests/test_pyupstart_system_init.py 2014-01-17 15:40:28 +0000
271@@ -61,6 +61,16 @@
272 version = self.upstart.version()
273 self.assertTrue(version)
274
275+ # Create an invalid job to ensure this causes no problems for
276+ # the re-exec. Note that we cannot use job_create() since
277+ # that validates the syntax of the .conf file).
278+ #
279+ # We create this file before any other to allow time for Upstart
280+ # to _attempt to parse it_ by the time the re-exec is initiated.
281+ invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
282+ with open(invalid_conf, 'w', encoding='utf-8') as fh:
283+ print("invalid", file=fh)
284+
285 # create a job and start it, marking it such that the .conf file
286 # will be retained when object becomes unusable (after re-exec).
287 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
288@@ -118,6 +128,8 @@
289 with self.assertRaises(ProcessLookupError):
290 os.kill(pid, 0)
291
292+ os.remove(invalid_conf)
293+
294 # Clean up
295 self.upstart.destroy()
296

Subscribers

People subscribed via source and target branches