Merge lp:~jamesodhunt/upstart/bug-1269731 into lp:upstart
- bug-1269731
- Merge into trunk
Proposed by
James Hunt
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+202083@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
- 1592. By James Hunt
-
* 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.
Revision history for this message
James Hunt (jamesodhunt) wrote : | # |
Revision history for this message
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 | 1 | 2014-01-17 James Hunt <james.hunt@ubuntu.com> | ||
6 | 2 | |||
7 | 3 | * init/conf.c: | ||
8 | 4 | - conf_file_serialise(): Handle ConfFile's without an | ||
9 | 5 | associated JobClass, resulting from an unparseable job configuration | ||
10 | 6 | file (LP: #1269731). | ||
11 | 7 | - conf_file_deserialise(): Comments. | ||
12 | 8 | * init/job_class.c: job_class_deserialise_all(): Comments. | ||
13 | 9 | * init/tests/test_conf.c: test_source_reload_file(): Add new tests: | ||
14 | 10 | - "Invalid .conf file does not stop ConfFile being serialised". | ||
15 | 11 | - "ConfFile with no JobClass can be deserialised". | ||
16 | 12 | * scripts/pyupstart.py: | ||
17 | 13 | - JobInstance:destroy(): Whitespace. | ||
18 | 14 | - SessionInit::reexec(): Add a log message. | ||
19 | 15 | * scripts/tests/test_pyupstart_session_init.py: | ||
20 | 16 | - TestSessionUpstart: Add pid to ps output. | ||
21 | 17 | - TestSessionInitReExec::test_session_init_reexec(): | ||
22 | 18 | - Create an invalid job to ensure re-exec can handle it. | ||
23 | 19 | - Use 'with' for handling re-exec exception. | ||
24 | 20 | * scripts/tests/test_pyupstart_system_init.py: | ||
25 | 21 | - TestSystemInitReExec::test_pid1_reexec(): | ||
26 | 22 | - Create an invalid job to ensure re-exec can handle it. | ||
27 | 23 | |||
28 | 1 | 2014-01-15 James Hunt <james.hunt@ubuntu.com> | 24 | 2014-01-15 James Hunt <james.hunt@ubuntu.com> |
29 | 2 | 25 | ||
30 | 3 | * util/telinit.c: upstart_open(): Connect using private socket | 26 | * util/telinit.c: upstart_open(): Connect using private socket |
31 | 4 | 27 | ||
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 | 1636 | if (! state_set_json_int_var_from_obj (json, file, flag)) | 1636 | if (! state_set_json_int_var_from_obj (json, file, flag)) |
37 | 1637 | goto error; | 1637 | goto error; |
38 | 1638 | 1638 | ||
39 | 1639 | if (! file->job) { | ||
40 | 1640 | /* File exists on disk but contains invalid | ||
41 | 1641 | * (unparseable) syntax, and hence no associated JobClass. | ||
42 | 1642 | * Thus, simply encode the ConfFile without a class. | ||
43 | 1643 | * | ||
44 | 1644 | * Deserialisation is handled automatically since | ||
45 | 1645 | * JobClasses are deserialised by directly iterating | ||
46 | 1646 | * through all JobClass'es found in the JSON. Here, | ||
47 | 1647 | * there simply won't be a JobClass to deserialise. | ||
48 | 1648 | */ | ||
49 | 1649 | goto out; | ||
50 | 1650 | } | ||
51 | 1651 | |||
52 | 1639 | /* | 1652 | /* |
53 | 1640 | * Ignore the "best" JobClass associated with this ConfFile | 1653 | * Ignore the "best" JobClass associated with this ConfFile |
54 | 1641 | * (file->job) since it won't be serialised. | 1654 | * (file->job) since it won't be serialised. |
55 | @@ -1681,6 +1694,7 @@ | |||
56 | 1681 | 1694 | ||
57 | 1682 | json_object_object_add (json, "job_class", json_job_class); | 1695 | json_object_object_add (json, "job_class", json_job_class); |
58 | 1683 | 1696 | ||
59 | 1697 | out: | ||
60 | 1684 | return json; | 1698 | return json; |
61 | 1685 | 1699 | ||
62 | 1686 | error: | 1700 | error: |
63 | @@ -1717,8 +1731,8 @@ | |||
64 | 1717 | goto error; | 1731 | goto error; |
65 | 1718 | 1732 | ||
66 | 1719 | /* Note that the associated JobClass is not handled at this | 1733 | /* Note that the associated JobClass is not handled at this |
69 | 1720 | * stage: it can't be the JobClasses haven't been deserialised | 1734 | * stage: it can't be since the JobClasses haven't been deserialised |
70 | 1721 | * yet. As such, the ConfFile->JobClass link is dealt with by | 1735 | * yet. As such, the ConfFile->JobClass link is dealt with in |
71 | 1722 | * job_class_deserialise_all(). | 1736 | * job_class_deserialise_all(). |
72 | 1723 | */ | 1737 | */ |
73 | 1724 | file->job = NULL; | 1738 | file->job = NULL; |
74 | 1725 | 1739 | ||
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 | 2429 | if (! state_check_json_type (json_class, object)) | 2429 | if (! state_check_json_type (json_class, object)) |
80 | 2430 | goto error; | 2430 | goto error; |
81 | 2431 | 2431 | ||
82 | 2432 | /* Responsible for associating a JobClass with its | ||
83 | 2433 | * parent ConfFile. | ||
84 | 2434 | */ | ||
85 | 2432 | class = job_class_deserialise (json_class); | 2435 | class = job_class_deserialise (json_class); |
86 | 2433 | 2436 | ||
87 | 2434 | /* Either memory is low or -- more likely -- a JobClass | 2437 | /* Either memory is low or -- more likely -- a JobClass |
88 | 2435 | 2438 | ||
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 | 3724 | void | 3724 | void |
94 | 3725 | test_source_reload_file (void) | 3725 | test_source_reload_file (void) |
95 | 3726 | { | 3726 | { |
104 | 3727 | ConfSource *source; | 3727 | ConfSource *source; |
105 | 3728 | ConfFile *file, *old_file; | 3728 | ConfFile *file, *old_file; |
106 | 3729 | FILE *f; | 3729 | FILE *f; |
107 | 3730 | int ret, fd, nfds; | 3730 | int ret, fd, nfds; |
108 | 3731 | char dirname[PATH_MAX]; | 3731 | char dirname[PATH_MAX]; |
109 | 3732 | char tmpname[PATH_MAX], filename[PATH_MAX]; | 3732 | char tmpname[PATH_MAX], filename[PATH_MAX]; |
110 | 3733 | fd_set readfds, writefds, exceptfds; | 3733 | fd_set readfds, writefds, exceptfds; |
111 | 3734 | NihError *err; | 3734 | NihError *err; |
112 | 3735 | json_object *json; | ||
113 | 3735 | 3736 | ||
114 | 3736 | TEST_FUNCTION_FEATURE ("conf_source_reload", | 3737 | TEST_FUNCTION_FEATURE ("conf_source_reload", |
115 | 3737 | "with configuration file"); | 3738 | "with configuration file"); |
116 | @@ -4500,12 +4501,76 @@ | |||
117 | 4500 | strcat (filename, "/bar.conf"); | 4501 | strcat (filename, "/bar.conf"); |
118 | 4501 | unlink (filename); | 4502 | unlink (filename); |
119 | 4502 | 4503 | ||
120 | 4504 | /* Re-enable inotify */ | ||
121 | 4505 | assert0 (unsetenv ("INOTIFY_DISABLE")); | ||
122 | 4506 | |||
123 | 4507 | TEST_FEATURE ("Invalid .conf file does not stop ConfFile being serialised"); | ||
124 | 4508 | conf_init (); | ||
125 | 4509 | TEST_LIST_EMPTY (conf_sources); | ||
126 | 4510 | |||
127 | 4511 | f = fopen (filename, "w"); | ||
128 | 4512 | /* Create an invalid job by adding an invalid stanza */ | ||
129 | 4513 | fprintf (f, "invalid\n"); | ||
130 | 4514 | fclose (f); | ||
131 | 4515 | |||
132 | 4516 | source = conf_source_new (NULL, filename, CONF_FILE); | ||
133 | 4517 | TEST_NE_P (source, NULL); | ||
134 | 4518 | TEST_LIST_NOT_EMPTY (conf_sources); | ||
135 | 4519 | TEST_HASH_EMPTY (source->files); | ||
136 | 4520 | |||
137 | 4521 | file = conf_file_new (source, "/path/to/file"); | ||
138 | 4522 | TEST_NE_P (file, NULL); | ||
139 | 4523 | TEST_HASH_NOT_EMPTY (source->files); | ||
140 | 4524 | |||
141 | 4525 | /* Initially, a ConfFile has no associated JobClass */ | ||
142 | 4526 | TEST_EQ_P (file->job, NULL); | ||
143 | 4527 | |||
144 | 4528 | /* Normally, this would create a JobClass and associate it with | ||
145 | 4529 | * its parent ConfFile, but that doesn't happen when the on-disk | ||
146 | 4530 | * job configuration file is invalid. | ||
147 | 4531 | */ | ||
148 | 4532 | ret = conf_source_reload (source); | ||
149 | 4533 | |||
150 | 4534 | /* Although the on-disk file is invalid, there is no error here | ||
151 | 4535 | * since it's already been handled by conf_reload_path() (which | ||
152 | 4536 | * displays an error message with details of how the job | ||
153 | 4537 | * configuration file is invalid). | ||
154 | 4538 | */ | ||
155 | 4539 | TEST_EQ (ret, 0); | ||
156 | 4540 | |||
157 | 4541 | /* We know the job was invalid * by the fact that the ConfFile | ||
158 | 4542 | * still has no associated JobClass. | ||
159 | 4543 | */ | ||
160 | 4544 | TEST_EQ (file->job, NULL); | ||
161 | 4545 | |||
162 | 4546 | /* See if we can serialise the ConfFile without an associated | ||
163 | 4547 | * JobClass. | ||
164 | 4548 | */ | ||
165 | 4549 | json = conf_file_serialise (file); | ||
166 | 4550 | TEST_NE_P (json, NULL); | ||
167 | 4551 | |||
168 | 4552 | /* Test there is no JobClass in the JSON */ | ||
169 | 4553 | TEST_EQ_P (json_object_object_get (json, "job_class"), NULL); | ||
170 | 4554 | |||
171 | 4555 | TEST_FEATURE ("ConfFile with no JobClass can be deserialised"); | ||
172 | 4556 | |||
173 | 4557 | nih_free (source); | ||
174 | 4558 | |||
175 | 4559 | source = conf_source_new (NULL, filename, CONF_FILE); | ||
176 | 4560 | TEST_NE_P (source, NULL); | ||
177 | 4561 | TEST_LIST_NOT_EMPTY (conf_sources); | ||
178 | 4562 | TEST_HASH_EMPTY (source->files); | ||
179 | 4563 | |||
180 | 4564 | file = conf_file_deserialise (source, json); | ||
181 | 4565 | TEST_NE_P (file, NULL); | ||
182 | 4566 | |||
183 | 4567 | nih_free (source); | ||
184 | 4568 | |||
185 | 4569 | json_object_put (json); | ||
186 | 4570 | |||
187 | 4571 | unlink (filename); | ||
188 | 4503 | rmdir (dirname); | 4572 | rmdir (dirname); |
189 | 4504 | |||
190 | 4505 | nih_log_set_priority (NIH_LOG_MESSAGE); | 4573 | nih_log_set_priority (NIH_LOG_MESSAGE); |
191 | 4506 | |||
192 | 4507 | /* Re-enable inotify */ | ||
193 | 4508 | assert0 (unsetenv ("INOTIFY_DISABLE")); | ||
194 | 4509 | } | 4574 | } |
195 | 4510 | 4575 | ||
196 | 4511 | 4576 | ||
197 | 4512 | 4577 | ||
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 | 925 | """ | 925 | """ |
203 | 926 | Stop the instance and cleanup. | 926 | Stop the instance and cleanup. |
204 | 927 | 927 | ||
207 | 928 | Note: If the instance specified retain when created, this will | 928 | Note: If the instance specified retain when created, this will |
208 | 929 | be a NOP. | 929 | be a NOP. |
209 | 930 | """ | 930 | """ |
210 | 931 | if not self.job.retain: | 931 | if not self.job.retain: |
211 | 932 | self.stop() | 932 | self.stop() |
212 | @@ -1119,4 +1119,5 @@ | |||
213 | 1119 | if not self.proxy: | 1119 | if not self.proxy: |
214 | 1120 | raise UpstartException('Not yet connected') | 1120 | raise UpstartException('Not yet connected') |
215 | 1121 | 1121 | ||
216 | 1122 | self.logger.debug("Restarting Session Init") | ||
217 | 1122 | self.proxy.Restart() | 1123 | self.proxy.Restart() |
218 | 1123 | 1124 | ||
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 | 51 | FILE_BRIDGE_CONF = 'upstart-file-bridge.conf' | 51 | FILE_BRIDGE_CONF = 'upstart-file-bridge.conf' |
224 | 52 | REEXEC_CONF = 're-exec.conf' | 52 | REEXEC_CONF = 're-exec.conf' |
225 | 53 | 53 | ||
227 | 54 | PSCMD_FMT = 'ps --no-headers -p %d -o comm,args' | 54 | PSCMD_FMT = 'ps --no-headers -p %d -o pid,comm,args' |
228 | 55 | 55 | ||
229 | 56 | def setUp(self): | 56 | def setUp(self): |
230 | 57 | 57 | ||
231 | @@ -343,6 +343,16 @@ | |||
232 | 343 | version = self.upstart.version() | 343 | version = self.upstart.version() |
233 | 344 | self.assertTrue(version) | 344 | self.assertTrue(version) |
234 | 345 | 345 | ||
235 | 346 | # Create an invalid job to ensure this causes no problems for | ||
236 | 347 | # the re-exec. Note that we cannot use job_create() since | ||
237 | 348 | # that validates the syntax of the .conf file). | ||
238 | 349 | # | ||
239 | 350 | # We create this file before any other to allow time for Upstart | ||
240 | 351 | # to _attempt to parse it_ by the time the re-exec is initiated. | ||
241 | 352 | invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir) | ||
242 | 353 | with open(invalid_conf, 'w', encoding='utf-8') as fh: | ||
243 | 354 | print("invalid", file=fh) | ||
244 | 355 | |||
245 | 346 | # create a job and start it, marking it such that the .conf file | 356 | # create a job and start it, marking it such that the .conf file |
246 | 347 | # will be retained when object becomes unusable (after re-exec). | 357 | # will be retained when object becomes unusable (after re-exec). |
247 | 348 | job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) | 358 | job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) |
248 | @@ -360,7 +370,8 @@ | |||
249 | 360 | # Trigger re-exec and catch the D-Bus exception resulting | 370 | # Trigger re-exec and catch the D-Bus exception resulting |
250 | 361 | # from disconnection from Session Init when it severs client | 371 | # from disconnection from Session Init when it severs client |
251 | 362 | # connections. | 372 | # connections. |
253 | 363 | self.assertRaises(dbus.exceptions.DBusException, self.upstart.reexec) | 373 | with self.assertRaises(dbus.exceptions.DBusException): |
254 | 374 | self.upstart.reexec() | ||
255 | 364 | 375 | ||
256 | 365 | os.kill(self.upstart.pid, 0) | 376 | os.kill(self.upstart.pid, 0) |
257 | 366 | 377 | ||
258 | @@ -411,6 +422,8 @@ | |||
259 | 411 | with self.assertRaises(ProcessLookupError): | 422 | with self.assertRaises(ProcessLookupError): |
260 | 412 | os.kill(pid, 0) | 423 | os.kill(pid, 0) |
261 | 413 | 424 | ||
262 | 425 | os.remove(invalid_conf) | ||
263 | 426 | |||
264 | 414 | self.stop_session_init() | 427 | self.stop_session_init() |
265 | 415 | 428 | ||
266 | 416 | def test_session_init_reexec_when_pid1_does(self): | 429 | def test_session_init_reexec_when_pid1_does(self): |
267 | 417 | 430 | ||
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 | 61 | version = self.upstart.version() | 61 | version = self.upstart.version() |
273 | 62 | self.assertTrue(version) | 62 | self.assertTrue(version) |
274 | 63 | 63 | ||
275 | 64 | # Create an invalid job to ensure this causes no problems for | ||
276 | 65 | # the re-exec. Note that we cannot use job_create() since | ||
277 | 66 | # that validates the syntax of the .conf file). | ||
278 | 67 | # | ||
279 | 68 | # We create this file before any other to allow time for Upstart | ||
280 | 69 | # to _attempt to parse it_ by the time the re-exec is initiated. | ||
281 | 70 | invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir) | ||
282 | 71 | with open(invalid_conf, 'w', encoding='utf-8') as fh: | ||
283 | 72 | print("invalid", file=fh) | ||
284 | 73 | |||
285 | 64 | # create a job and start it, marking it such that the .conf file | 74 | # create a job and start it, marking it such that the .conf file |
286 | 65 | # will be retained when object becomes unusable (after re-exec). | 75 | # will be retained when object becomes unusable (after re-exec). |
287 | 66 | job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) | 76 | job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) |
288 | @@ -118,6 +128,8 @@ | |||
289 | 118 | with self.assertRaises(ProcessLookupError): | 128 | with self.assertRaises(ProcessLookupError): |
290 | 119 | os.kill(pid, 0) | 129 | os.kill(pid, 0) |
291 | 120 | 130 | ||
292 | 131 | os.remove(invalid_conf) | ||
293 | 132 | |||
294 | 121 | # Clean up | 133 | # Clean up |
295 | 122 | self.upstart.destroy() | 134 | self.upstart.destroy() |
296 | 123 | 135 |
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.