Merge authors: James Hunt (jamesodhunt) Related merge proposals: https://code.launchpad.net/~jamesodhunt/upstart/bug-1269731/+merge/202083 proposed by: James Hunt (jamesodhunt) review: Approve - Colin Watson (cjwatson) ------------------------------------------------------------ revno: 1591 [merge] committer: James Hunt <[email protected]> branch nick: upstart timestamp: Tue 2014-01-21 10:36:24 +0000 message: * Merge of lp:~jamesodhunt/upstart/bug-1269731. modified: ChangeLog init/conf.c init/job_class.c init/tests/test_conf.c scripts/pyupstart.py scripts/tests/test_pyupstart_session_init.py scripts/tests/test_pyupstart_system_init.py
-- lp:upstart https://code.launchpad.net/~upstart-devel/upstart/trunk Your team Upstart Reviewers is subscribed to branch lp:upstart. To unsubscribe from this branch go to https://code.launchpad.net/~upstart-devel/upstart/trunk/+edit-subscription
=== modified file 'ChangeLog' --- ChangeLog 2014-01-15 11:51:18 +0000 +++ ChangeLog 2014-01-17 15:39:55 +0000 @@ -1,3 +1,26 @@ +2014-01-17 James Hunt <[email protected]> + + * init/conf.c: + - conf_file_serialise(): Handle ConfFile's without an + associated JobClass, resulting from an unparseable job configuration + file (LP: #1269731). + - conf_file_deserialise(): Comments. + * init/job_class.c: job_class_deserialise_all(): Comments. + * init/tests/test_conf.c: test_source_reload_file(): Add new tests: + - "Invalid .conf file does not stop ConfFile being serialised". + - "ConfFile with no JobClass can be deserialised". + * 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. + 2014-01-15 James Hunt <[email protected]> * util/telinit.c: upstart_open(): Connect using private socket === modified file 'init/conf.c' --- init/conf.c 2013-11-16 12:42:57 +0000 +++ init/conf.c 2014-01-17 12:03:24 +0000 @@ -1636,6 +1636,19 @@ if (! state_set_json_int_var_from_obj (json, file, flag)) goto error; + if (! file->job) { + /* File exists on disk but contains invalid + * (unparseable) syntax, and hence no associated JobClass. + * Thus, simply encode the ConfFile without a class. + * + * Deserialisation is handled automatically since + * JobClasses are deserialised by directly iterating + * through all JobClass'es found in the JSON. Here, + * there simply won't be a JobClass to deserialise. + */ + goto out; + } + /* * Ignore the "best" JobClass associated with this ConfFile * (file->job) since it won't be serialised. @@ -1681,6 +1694,7 @@ json_object_object_add (json, "job_class", json_job_class); +out: return json; error: @@ -1717,8 +1731,8 @@ goto error; /* Note that the associated JobClass is not handled at this - * stage: it can't be the JobClasses haven't been deserialised - * yet. As such, the ConfFile->JobClass link is dealt with by + * stage: it can't be since the JobClasses haven't been deserialised + * yet. As such, the ConfFile->JobClass link is dealt with in * job_class_deserialise_all(). */ file->job = NULL; === modified file 'init/job_class.c' --- init/job_class.c 2013-11-04 09:34:51 +0000 +++ init/job_class.c 2014-01-17 12:03:24 +0000 @@ -2429,6 +2429,9 @@ if (! state_check_json_type (json_class, object)) goto error; + /* Responsible for associating a JobClass with its + * parent ConfFile. + */ class = job_class_deserialise (json_class); /* Either memory is low or -- more likely -- a JobClass === 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 12:03:24 +0000 @@ -3724,14 +3724,15 @@ void test_source_reload_file (void) { - ConfSource *source; - ConfFile *file, *old_file; - FILE *f; - int ret, fd, nfds; - char dirname[PATH_MAX]; - char tmpname[PATH_MAX], filename[PATH_MAX]; - fd_set readfds, writefds, exceptfds; - NihError *err; + ConfSource *source; + ConfFile *file, *old_file; + FILE *f; + int ret, fd, nfds; + char dirname[PATH_MAX]; + char tmpname[PATH_MAX], filename[PATH_MAX]; + fd_set readfds, writefds, exceptfds; + NihError *err; + json_object *json; TEST_FUNCTION_FEATURE ("conf_source_reload", "with configuration file"); @@ -4500,12 +4501,76 @@ strcat (filename, "/bar.conf"); unlink (filename); + /* Re-enable inotify */ + assert0 (unsetenv ("INOTIFY_DISABLE")); + + TEST_FEATURE ("Invalid .conf file does not stop ConfFile being serialised"); + conf_init (); + TEST_LIST_EMPTY (conf_sources); + + f = fopen (filename, "w"); + /* Create an invalid job by adding an invalid stanza */ + fprintf (f, "invalid\n"); + fclose (f); + + source = conf_source_new (NULL, filename, CONF_FILE); + TEST_NE_P (source, NULL); + TEST_LIST_NOT_EMPTY (conf_sources); + TEST_HASH_EMPTY (source->files); + + file = conf_file_new (source, "/path/to/file"); + TEST_NE_P (file, NULL); + TEST_HASH_NOT_EMPTY (source->files); + + /* Initially, a ConfFile has no associated JobClass */ + TEST_EQ_P (file->job, NULL); + + /* Normally, this would create a JobClass and associate it with + * its parent ConfFile, but that doesn't happen when the on-disk + * job configuration file is invalid. + */ + ret = conf_source_reload (source); + + /* Although the on-disk file is invalid, there is no error here + * since it's already been handled by conf_reload_path() (which + * displays an error message with details of how the job + * configuration file is invalid). + */ + TEST_EQ (ret, 0); + + /* We know the job was invalid * by the fact that the ConfFile + * still has no associated JobClass. + */ + TEST_EQ (file->job, NULL); + + /* See if we can serialise the ConfFile without an associated + * JobClass. + */ + json = conf_file_serialise (file); + TEST_NE_P (json, NULL); + + /* Test there is no JobClass in the JSON */ + TEST_EQ_P (json_object_object_get (json, "job_class"), NULL); + + TEST_FEATURE ("ConfFile with no JobClass can be deserialised"); + + nih_free (source); + + source = conf_source_new (NULL, filename, CONF_FILE); + TEST_NE_P (source, NULL); + TEST_LIST_NOT_EMPTY (conf_sources); + TEST_HASH_EMPTY (source->files); + + file = conf_file_deserialise (source, json); + TEST_NE_P (file, NULL); + + nih_free (source); + + json_object_put (json); + + unlink (filename); rmdir (dirname); - nih_log_set_priority (NIH_LOG_MESSAGE); - - /* Re-enable inotify */ - assert0 (unsetenv ("INOTIFY_DISABLE")); } === modified file 'scripts/pyupstart.py' --- scripts/pyupstart.py 2013-09-12 10:56:22 +0000 +++ scripts/pyupstart.py 2014-01-17 15:39:55 +0000 @@ -925,8 +925,8 @@ """ Stop the instance and cleanup. - Note: If the instance specified retain when created, this will - be a NOP. + Note: If the instance specified retain when created, this will + be a NOP. """ if not self.job.retain: self.stop() @@ -1119,4 +1119,5 @@ if not self.proxy: raise UpstartException('Not yet connected') + self.logger.debug("Restarting Session Init") self.proxy.Restart() === 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:39:55 +0000 @@ -51,7 +51,7 @@ FILE_BRIDGE_CONF = 'upstart-file-bridge.conf' REEXEC_CONF = 're-exec.conf' - PSCMD_FMT = 'ps --no-headers -p %d -o comm,args' + PSCMD_FMT = 'ps --no-headers -p %d -o pid,comm,args' def setUp(self): @@ -343,6 +343,16 @@ version = self.upstart.version() self.assertTrue(version) + # Create an invalid job to ensure this causes no problems for + # the re-exec. Note that we cannot use job_create() since + # that validates the syntax of the .conf file). + # + # We create this file before any other to allow time for Upstart + # to _attempt to parse it_ by the time the re-exec is initiated. + invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir) + with open(invalid_conf, 'w', encoding='utf-8') as fh: + print("invalid", file=fh) + # create a job and start it, marking it such that the .conf file # will be retained when object becomes unusable (after re-exec). job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) @@ -360,7 +370,8 @@ # Trigger re-exec and catch the D-Bus exception resulting # from disconnection from Session Init when it severs client # connections. - self.assertRaises(dbus.exceptions.DBusException, self.upstart.reexec) + with self.assertRaises(dbus.exceptions.DBusException): + self.upstart.reexec() os.kill(self.upstart.pid, 0) @@ -411,6 +422,8 @@ with self.assertRaises(ProcessLookupError): os.kill(pid, 0) + os.remove(invalid_conf) + self.stop_session_init() def test_session_init_reexec_when_pid1_does(self): === 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:39:55 +0000 @@ -61,6 +61,16 @@ version = self.upstart.version() self.assertTrue(version) + # Create an invalid job to ensure this causes no problems for + # the re-exec. Note that we cannot use job_create() since + # that validates the syntax of the .conf file). + # + # We create this file before any other to allow time for Upstart + # to _attempt to parse it_ by the time the re-exec is initiated. + invalid_conf = "{}/invalid.conf".format(self.upstart.test_dir) + with open(invalid_conf, 'w', encoding='utf-8') as fh: + print("invalid", file=fh) + # create a job and start it, marking it such that the .conf file # will be retained when object becomes unusable (after re-exec). job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True) @@ -118,6 +128,8 @@ with self.assertRaises(ProcessLookupError): os.kill(pid, 0) + os.remove(invalid_conf) + # Clean up self.upstart.destroy()
-- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
