[Merge] lp:~jamesodhunt/upstart/setenv+getenv into lp:upstart

[Steve Langasek]

Review: Needs Fixing

There seems to be some duplication in the changelog (the entries for 2012-12-11 and 2012-12-17).  Cut'n'paste error?

> _("Not permissable to modify PID 1 job environment"));

Nitpick: "permissible"

+ if (control_get_origin_uid (message, &origin_uid) && origin_uid != uid) {

Same concern as on the other branch: this should fail closed when control_get_origin_uid() breaks.

Shouldn't init/control.h include a full set of prototypes for the new functions?  Currently it only includes control_{get,set,list}_env; control_{reset,unset}_env aren't listed.  I would expect this to result in compiler warnings, if not outright errors on 64-bit archs.

There seem to be some excessive calls to job_class_environment_init(): not only is it being called from each of the job_class_environment_* functions, it's also being called from main(), *and* from job_class_environment().  I think this should be tightened up to skip the redundant initialization checks - we should really only need to call this once at program startup, and then from job_class_environment_reset() to reset the array.  Presumably the call in init/main.c is the one that should be retained, as this ensures it's initialized before any dbus calls try to modify it.

+ RUN_COMMAND (NULL, cmd, &output, &line_count);
+
+ TEST_STR_MATCH (output[0], "PATH=*");
+ TEST_STR_MATCH (output[1], "TERM=*");
+ TEST_EQ (line_count, 2);

In the unlikely event that the command fails to produce any output, it looks like this could cause a segfault... the line_count should probably be checked first, before poking at the output array so that we can at least debug the failing test.

+ /* don't check the actual value (in case user has changed it from
+ * default value when compiling), just see if it matches a
+ * reasonable pattern.
+ */
+ TEST_EQ_STR (output[0], getenv ("TERM"));
+ TEST_EQ (line_count, 1);
+ nih_free (output);

This comment is inaccurate - seems to be cut'n'pasted from the PATH check, where we are doing as described.  But why not use getenv("PATH") for that check too?

At a higher level, these interfaces introduce support for modifying not just the environment for future jobs, but also the environment of a particular job instance.  https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions doesn't discuss why this is needed, but I am concerned about the potential for incorrect use here.  If the environment of a job instance is changed after it's already started, the stop scripts will obviously have a different environment than the start scripts did - but this may not be at all obvious to the user.  Are you sure we want to open ourselves up to all the various sorts of foot shooting that may result here?  At the very least, if the per-job interface is needed please update the wiki page with an explanation as to why.

-- 
https://code.launchpad.net/~jamesodhunt/upstart/setenv+getenv/+merge/145548
Your team Upstart Reviewers is subscribed to branch lp:upstart.