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

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
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+202083@code.launchpad.net
To post a comment you must log in.
lp:~jamesodhunt/upstart/bug-1269731 updated
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 :

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.

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
=== modified file 'ChangeLog'
--- ChangeLog 2014-01-15 11:51:18 +0000
+++ ChangeLog 2014-01-17 15:40:28 +0000
@@ -1,3 +1,26 @@
12014-01-17 James Hunt <james.hunt@ubuntu.com>
2
3 * init/conf.c:
4 - conf_file_serialise(): Handle ConfFile's without an
5 associated JobClass, resulting from an unparseable job configuration
6 file (LP: #1269731).
7 - conf_file_deserialise(): Comments.
8 * init/job_class.c: job_class_deserialise_all(): Comments.
9 * init/tests/test_conf.c: test_source_reload_file(): Add new tests:
10 - "Invalid .conf file does not stop ConfFile being serialised".
11 - "ConfFile with no JobClass can be deserialised".
12 * scripts/pyupstart.py:
13 - JobInstance:destroy(): Whitespace.
14 - SessionInit::reexec(): Add a log message.
15 * scripts/tests/test_pyupstart_session_init.py:
16 - TestSessionUpstart: Add pid to ps output.
17 - TestSessionInitReExec::test_session_init_reexec():
18 - Create an invalid job to ensure re-exec can handle it.
19 - Use 'with' for handling re-exec exception.
20 * scripts/tests/test_pyupstart_system_init.py:
21 - TestSystemInitReExec::test_pid1_reexec():
22 - Create an invalid job to ensure re-exec can handle it.
23
12014-01-15 James Hunt <james.hunt@ubuntu.com>242014-01-15 James Hunt <james.hunt@ubuntu.com>
225
3 * util/telinit.c: upstart_open(): Connect using private socket26 * util/telinit.c: upstart_open(): Connect using private socket
427
=== modified file 'init/conf.c'
--- init/conf.c 2013-11-16 12:42:57 +0000
+++ init/conf.c 2014-01-17 15:40:28 +0000
@@ -1636,6 +1636,19 @@
1636 if (! state_set_json_int_var_from_obj (json, file, flag))1636 if (! state_set_json_int_var_from_obj (json, file, flag))
1637 goto error;1637 goto error;
16381638
1639 if (! file->job) {
1640 /* File exists on disk but contains invalid
1641 * (unparseable) syntax, and hence no associated JobClass.
1642 * Thus, simply encode the ConfFile without a class.
1643 *
1644 * Deserialisation is handled automatically since
1645 * JobClasses are deserialised by directly iterating
1646 * through all JobClass'es found in the JSON. Here,
1647 * there simply won't be a JobClass to deserialise.
1648 */
1649 goto out;
1650 }
1651
1639 /*1652 /*
1640 * Ignore the "best" JobClass associated with this ConfFile1653 * Ignore the "best" JobClass associated with this ConfFile
1641 * (file->job) since it won't be serialised.1654 * (file->job) since it won't be serialised.
@@ -1681,6 +1694,7 @@
16811694
1682 json_object_object_add (json, "job_class", json_job_class);1695 json_object_object_add (json, "job_class", json_job_class);
16831696
1697out:
1684 return json;1698 return json;
16851699
1686error:1700error:
@@ -1717,8 +1731,8 @@
1717 goto error;1731 goto error;
17181732
1719 /* Note that the associated JobClass is not handled at this1733 /* Note that the associated JobClass is not handled at this
1720 * stage: it can't be the JobClasses haven't been deserialised1734 * stage: it can't be since the JobClasses haven't been deserialised
1721 * yet. As such, the ConfFile->JobClass link is dealt with by1735 * yet. As such, the ConfFile->JobClass link is dealt with in
1722 * job_class_deserialise_all().1736 * job_class_deserialise_all().
1723 */1737 */
1724 file->job = NULL;1738 file->job = NULL;
17251739
=== modified file 'init/job_class.c'
--- init/job_class.c 2013-11-04 09:34:51 +0000
+++ init/job_class.c 2014-01-17 15:40:28 +0000
@@ -2429,6 +2429,9 @@
2429 if (! state_check_json_type (json_class, object))2429 if (! state_check_json_type (json_class, object))
2430 goto error;2430 goto error;
24312431
2432 /* Responsible for associating a JobClass with its
2433 * parent ConfFile.
2434 */
2432 class = job_class_deserialise (json_class);2435 class = job_class_deserialise (json_class);
24332436
2434 /* Either memory is low or -- more likely -- a JobClass2437 /* Either memory is low or -- more likely -- a JobClass
24352438
=== modified file 'init/tests/test_conf.c'
--- init/tests/test_conf.c 2013-06-25 10:13:12 +0000
+++ init/tests/test_conf.c 2014-01-17 15:40:28 +0000
@@ -3724,14 +3724,15 @@
3724void3724void
3725test_source_reload_file (void)3725test_source_reload_file (void)
3726{3726{
3727 ConfSource *source;3727 ConfSource *source;
3728 ConfFile *file, *old_file;3728 ConfFile *file, *old_file;
3729 FILE *f;3729 FILE *f;
3730 int ret, fd, nfds;3730 int ret, fd, nfds;
3731 char dirname[PATH_MAX];3731 char dirname[PATH_MAX];
3732 char tmpname[PATH_MAX], filename[PATH_MAX];3732 char tmpname[PATH_MAX], filename[PATH_MAX];
3733 fd_set readfds, writefds, exceptfds;3733 fd_set readfds, writefds, exceptfds;
3734 NihError *err;3734 NihError *err;
3735 json_object *json;
37353736
3736 TEST_FUNCTION_FEATURE ("conf_source_reload",3737 TEST_FUNCTION_FEATURE ("conf_source_reload",
3737 "with configuration file");3738 "with configuration file");
@@ -4500,12 +4501,76 @@
4500 strcat (filename, "/bar.conf");4501 strcat (filename, "/bar.conf");
4501 unlink (filename);4502 unlink (filename);
45024503
4504 /* Re-enable inotify */
4505 assert0 (unsetenv ("INOTIFY_DISABLE"));
4506
4507 TEST_FEATURE ("Invalid .conf file does not stop ConfFile being serialised");
4508 conf_init ();
4509 TEST_LIST_EMPTY (conf_sources);
4510
4511 f = fopen (filename, "w");
4512 /* Create an invalid job by adding an invalid stanza */
4513 fprintf (f, "invalid\n");
4514 fclose (f);
4515
4516 source = conf_source_new (NULL, filename, CONF_FILE);
4517 TEST_NE_P (source, NULL);
4518 TEST_LIST_NOT_EMPTY (conf_sources);
4519 TEST_HASH_EMPTY (source->files);
4520
4521 file = conf_file_new (source, "/path/to/file");
4522 TEST_NE_P (file, NULL);
4523 TEST_HASH_NOT_EMPTY (source->files);
4524
4525 /* Initially, a ConfFile has no associated JobClass */
4526 TEST_EQ_P (file->job, NULL);
4527
4528 /* Normally, this would create a JobClass and associate it with
4529 * its parent ConfFile, but that doesn't happen when the on-disk
4530 * job configuration file is invalid.
4531 */
4532 ret = conf_source_reload (source);
4533
4534 /* Although the on-disk file is invalid, there is no error here
4535 * since it's already been handled by conf_reload_path() (which
4536 * displays an error message with details of how the job
4537 * configuration file is invalid).
4538 */
4539 TEST_EQ (ret, 0);
4540
4541 /* We know the job was invalid * by the fact that the ConfFile
4542 * still has no associated JobClass.
4543 */
4544 TEST_EQ (file->job, NULL);
4545
4546 /* See if we can serialise the ConfFile without an associated
4547 * JobClass.
4548 */
4549 json = conf_file_serialise (file);
4550 TEST_NE_P (json, NULL);
4551
4552 /* Test there is no JobClass in the JSON */
4553 TEST_EQ_P (json_object_object_get (json, "job_class"), NULL);
4554
4555 TEST_FEATURE ("ConfFile with no JobClass can be deserialised");
4556
4557 nih_free (source);
4558
4559 source = conf_source_new (NULL, filename, CONF_FILE);
4560 TEST_NE_P (source, NULL);
4561 TEST_LIST_NOT_EMPTY (conf_sources);
4562 TEST_HASH_EMPTY (source->files);
4563
4564 file = conf_file_deserialise (source, json);
4565 TEST_NE_P (file, NULL);
4566
4567 nih_free (source);
4568
4569 json_object_put (json);
4570
4571 unlink (filename);
4503 rmdir (dirname);4572 rmdir (dirname);
4504
4505 nih_log_set_priority (NIH_LOG_MESSAGE);4573 nih_log_set_priority (NIH_LOG_MESSAGE);
4506
4507 /* Re-enable inotify */
4508 assert0 (unsetenv ("INOTIFY_DISABLE"));
4509}4574}
45104575
45114576
45124577
=== modified file 'scripts/pyupstart.py'
--- scripts/pyupstart.py 2013-09-12 10:56:22 +0000
+++ scripts/pyupstart.py 2014-01-17 15:40:28 +0000
@@ -925,8 +925,8 @@
925 """925 """
926 Stop the instance and cleanup.926 Stop the instance and cleanup.
927927
928 Note: If the instance specified retain when created, this will928 Note: If the instance specified retain when created, this will
929 be a NOP.929 be a NOP.
930 """930 """
931 if not self.job.retain:931 if not self.job.retain:
932 self.stop()932 self.stop()
@@ -1119,4 +1119,5 @@
1119 if not self.proxy:1119 if not self.proxy:
1120 raise UpstartException('Not yet connected')1120 raise UpstartException('Not yet connected')
11211121
1122 self.logger.debug("Restarting Session Init")
1122 self.proxy.Restart()1123 self.proxy.Restart()
11231124
=== modified file 'scripts/tests/test_pyupstart_session_init.py'
--- scripts/tests/test_pyupstart_session_init.py 2013-11-03 00:28:07 +0000
+++ scripts/tests/test_pyupstart_session_init.py 2014-01-17 15:40:28 +0000
@@ -51,7 +51,7 @@
51 FILE_BRIDGE_CONF = 'upstart-file-bridge.conf'51 FILE_BRIDGE_CONF = 'upstart-file-bridge.conf'
52 REEXEC_CONF = 're-exec.conf'52 REEXEC_CONF = 're-exec.conf'
5353
54 PSCMD_FMT = 'ps --no-headers -p %d -o comm,args'54 PSCMD_FMT = 'ps --no-headers -p %d -o pid,comm,args'
5555
56 def setUp(self):56 def setUp(self):
5757
@@ -343,6 +343,16 @@
343 version = self.upstart.version()343 version = self.upstart.version()
344 self.assertTrue(version)344 self.assertTrue(version)
345345
346 # Create an invalid job to ensure this causes no problems for
347 # the re-exec. Note that we cannot use job_create() since
348 # that validates the syntax of the .conf file).
349 #
350 # We create this file before any other to allow time for Upstart
351 # to _attempt to parse it_ by the time the re-exec is initiated.
352 invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
353 with open(invalid_conf, 'w', encoding='utf-8') as fh:
354 print("invalid", file=fh)
355
346 # create a job and start it, marking it such that the .conf file356 # create a job and start it, marking it such that the .conf file
347 # will be retained when object becomes unusable (after re-exec).357 # will be retained when object becomes unusable (after re-exec).
348 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)358 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
@@ -360,7 +370,8 @@
360 # Trigger re-exec and catch the D-Bus exception resulting370 # Trigger re-exec and catch the D-Bus exception resulting
361 # from disconnection from Session Init when it severs client371 # from disconnection from Session Init when it severs client
362 # connections.372 # connections.
363 self.assertRaises(dbus.exceptions.DBusException, self.upstart.reexec)373 with self.assertRaises(dbus.exceptions.DBusException):
374 self.upstart.reexec()
364375
365 os.kill(self.upstart.pid, 0)376 os.kill(self.upstart.pid, 0)
366377
@@ -411,6 +422,8 @@
411 with self.assertRaises(ProcessLookupError):422 with self.assertRaises(ProcessLookupError):
412 os.kill(pid, 0)423 os.kill(pid, 0)
413424
425 os.remove(invalid_conf)
426
414 self.stop_session_init()427 self.stop_session_init()
415428
416 def test_session_init_reexec_when_pid1_does(self):429 def test_session_init_reexec_when_pid1_does(self):
417430
=== modified file 'scripts/tests/test_pyupstart_system_init.py'
--- scripts/tests/test_pyupstart_system_init.py 2013-09-12 10:56:22 +0000
+++ scripts/tests/test_pyupstart_system_init.py 2014-01-17 15:40:28 +0000
@@ -61,6 +61,16 @@
61 version = self.upstart.version()61 version = self.upstart.version()
62 self.assertTrue(version)62 self.assertTrue(version)
6363
64 # Create an invalid job to ensure this causes no problems for
65 # the re-exec. Note that we cannot use job_create() since
66 # that validates the syntax of the .conf file).
67 #
68 # We create this file before any other to allow time for Upstart
69 # to _attempt to parse it_ by the time the re-exec is initiated.
70 invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir)
71 with open(invalid_conf, 'w', encoding='utf-8') as fh:
72 print("invalid", file=fh)
73
64 # create a job and start it, marking it such that the .conf file74 # create a job and start it, marking it such that the .conf file
65 # will be retained when object becomes unusable (after re-exec).75 # will be retained when object becomes unusable (after re-exec).
66 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)76 job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
@@ -118,6 +128,8 @@
118 with self.assertRaises(ProcessLookupError):128 with self.assertRaises(ProcessLookupError):
119 os.kill(pid, 0)129 os.kill(pid, 0)
120130
131 os.remove(invalid_conf)
132
121 # Clean up133 # Clean up
122 self.upstart.destroy()134 self.upstart.destroy()
123135

Subscribers

People subscribed via source and target branches