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.
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
