Review: Approve
Looks good to me, with below mentioned included cleanups.
Diff comments:
> === modified file 'ChangeLog'
> --- ChangeLog 2014-07-03 08:59:30 +0000
> +++ ChangeLog 2014-07-07 10:15:49 +0000
> @@ -1,3 +1,10 @@
> +2014-07-07 James Hunt <james.h...@ubuntu.com>
> +
> + * init/tests/test_job_process.c: test_handler():
> + - Remove logs generated by PROCESS_MAIN. This wasn't necessary before
> + due to the manifestation of LP: #1302117 not restoring the correct
> + umask.
> +
> 2014-07-03 James Hunt <james.h...@ubuntu.com>
>
> * init/job_process.c:
> @@ -97,6 +104,13 @@
> - Clarify default behaviour of 'respawn' stanza.
> - Add missing 'respawn limit unlimited' details.
>
> +2014-04-10 James Hunt <james.h...@ubuntu.com>
> +
> + * init/log.c: log_file_open(): Restore umask after opening log file
> + (LP: #1302117).
> + * util/tests/test_initctl.c: test_reexec(): New test:
> + - "ensure re-exec does not disrupt umask".
> +
> 2014-03-11 James Hunt <james.h...@ubuntu.com>
>
> * NEWS: Release 1.12.1
>
> === modified file 'init/log.c'
> --- init/log.c 2014-06-04 17:37:59 +0000
> +++ init/log.c 2014-07-07 10:15:49 +0000
> @@ -405,6 +405,9 @@
> struct stat statbuf;
> int ret = -1;
> int mode = LOG_DEFAULT_MODE;
> +#if 1
> + mode_t old;
> +#endif
> int flags = (O_CREAT | O_APPEND | O_WRONLY |
I have cleaned up this #if...
> O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK);
>
> @@ -439,7 +442,11 @@
> nih_assert (log->fd == -1);
>
> /* Impose some sane defaults. */
> +#if 1
> + old = umask (LOG_DEFAULT_UMASK);
> +#else
> umask (LOG_DEFAULT_UMASK);
> +#endif
>
... and this one ...
> /* Non-blocking to avoid holding up the main loop. Without
> * this, we'd probably need to spawn a thread to handle
> @@ -447,6 +454,11 @@
> */
> log->fd = open (log->path, flags, mode);
>
> + /* Restore */
> +#if 1
> + umask (old);
> +#endif
> +
> log->open_errno = errno;
... and this one.
>
> /* Open may have failed due to path being unaccessible
>
> === modified file 'init/tests/test_job_process.c'
> --- init/tests/test_job_process.c 2014-06-05 17:57:29 +0000
> +++ init/tests/test_job_process.c 2014-07-07 10:15:49 +0000
> @@ -1,11 +1,8 @@
> -/*
> - * FIXME: test_job_process has been left behind
> - */
> /* upstart
> *
> * test_job_process.c - test suite for init/job_process.c
> *
> - * Copyright 2011 Canonical Ltd.
> + * Copyright © 2011-2014 Canonical Ltd.
> * Author: Scott James Remnant <sc...@netsplit.com>.
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -5489,11 +5486,16 @@
> unsigned long data;
> struct timespec now;
> char dirname[PATH_MAX];
> + nih_local char *logfile = NULL;
> + int fds[2] = { -1, -1};
>
> TEST_FILENAME (dirname);
> TEST_EQ (mkdir (dirname, 0755), 0);
> TEST_EQ (setenv ("UPSTART_LOGDIR", dirname, 1), 0);
>
> + logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s.log",
> + dirname, "test"));
> +
> TEST_FUNCTION ("job_process_handler");
> program_name = "test";
> output = tmpfile ();
> @@ -5763,6 +5765,9 @@
> class->process[PROCESS_PRE_START] = process_new (class);
> class->process[PROCESS_PRE_START]->command = "echo";
>
> + assert0 (pipe (fds));
> + close (fds[1]);
> +
> TEST_ALLOC_FAIL {
> TEST_ALLOC_SAFE {
> job = job_new (class, "");
> @@ -5770,6 +5775,9 @@
> blocked = blocked_new (job, BLOCKED_EVENT, event);
> event_block (event);
> nih_list_add (&job->blocking, &blocked->entry);
> +
> + job->process_data[PROCESS_PRE_START] = NIH_MUST (
> + job_process_data_new
> (job->process_data, job, PROCESS_PRE_START, fds[0]));
> }
>
> job->goal = JOB_START;
> @@ -5812,9 +5820,12 @@
> nih_free (job);
> }
>
> + close (fds[0]);
> +
> nih_free (class->process[PROCESS_PRE_START]);
> class->process[PROCESS_PRE_START] = NULL;
>
> + unlink (logfile);
>
> /* Check that we can handle a failing pre-start process of the job,
> * which changes the goal to stop and transitions a state change in
> @@ -5890,6 +5901,7 @@
> nih_free (class->process[PROCESS_PRE_START]);
> class->process[PROCESS_PRE_START] = NULL;
>
> + unlink (logfile);
>
> /* Check that we can handle a killed starting task, which should
> * act as if it failed. A different error should be output and
> @@ -5964,6 +5976,7 @@
> nih_free (class->process[PROCESS_PRE_START]);
> class->process[PROCESS_PRE_START] = NULL;
>
> + unlink (logfile);
>
> /* Check that we can catch the running task of a service stopping
> * with an error, and if the job is to be respawned, go into
>
> === modified file 'util/tests/test_initctl.c'
> --- util/tests/test_initctl.c 2014-06-05 10:13:51 +0000
> +++ util/tests/test_initctl.c 2014-07-07 10:15:49 +0000
> @@ -11052,6 +11052,8 @@
> FILE *file;
> int ok;
> int ret;
> + mode_t expected_umask;
> + size_t len;
>
> TEST_GROUP ("re-exec support");
>
> @@ -11317,6 +11319,80 @@
> STOP_UPSTART (upstart_pid);
>
> /*******************************************************************/
> + TEST_FEATURE ("ensure re-exec does not disrupt umask");
> +
> + contents = nih_sprintf (NULL, "exec sh -c umask");
> + TEST_NE_P (contents, NULL);
> +
> + CREATE_FILE (confdir, "umask.conf", contents);
> + nih_free (contents);
> +
> + start_upstart_common (&upstart_pid, TRUE, TRUE, confdir, logdir, NULL);
> +
> + cmd = nih_sprintf (NULL, "%s start umask 2>&1", get_initctl ());
> + TEST_NE_P (cmd, NULL);
> + RUN_COMMAND (NULL, cmd, &output, &lines);
> + nih_free (output);
> +
> + logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
> + logdir,
> + "umask.log"));
> +
> + /* Wait for log to be created */
> + ok = FALSE;
> + for (int i = 0; i < 5; i++) {
> + sleep (1);
> + if (! stat (logfile, &statbuf)) {
> + ok = TRUE;
> + break;
> + }
> + }
> + TEST_EQ (ok, TRUE);
> +
> + contents = nih_file_read (NULL, logfile, &len);
> + TEST_NE_P (contents, NULL);
> + TEST_TRUE (len);
> +
> + /* overwrite '\n' */
> + contents[len-1] = '\0';
> +
> + expected_umask = (mode_t)atoi (contents);
> + assert0 (unlink (logfile));
> + nih_free (contents);
> +
> + /* Restart */
> + REEXEC_UPSTART (upstart_pid, TRUE);
> +
> + /* Re-run job */
> + RUN_COMMAND (NULL, cmd, &output, &lines);
> + nih_free (output);
> +
> + /* Wait for log to be recreated */
> + ok = FALSE;
> + for (int i = 0; i < 5; i++) {
> + sleep (1);
> + if (! stat (logfile, &statbuf)) {
> + ok = TRUE;
> + break;
> + }
> + }
> + TEST_EQ (ok, TRUE);
> +
> + contents = nih_file_read (NULL, logfile, &len);
> + TEST_NE_P (contents, NULL);
> + TEST_TRUE (len);
> +
> + /* overwrite '\n' */
> + contents[len-1] = '\0';
> +
> + TEST_EQ (expected_umask, (mode_t)atoi (contents));
> +
> + STOP_UPSTART (upstart_pid);
> +
> + DELETE_FILE (confdir, "umask.conf");
> + assert0 (unlink (logfile));
> +
> + /*******************************************************************/
>
> TEST_DBUS_END (dbus_pid);
>
>
--
https://code.launchpad.net/~jamesodhunt/upstart/bug-1302117-remerged/+merge/225177
Your team Upstart Reviewers is subscribed to branch lp:upstart.
--
upstart-devel mailing list
upstart-devel@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
