Code review comment for lp:~jamesodhunt/upstart/bug-1083723

Revision history for this message
Steve Langasek (vorlon) wrote :

+ /* Disallow users from restarting Upstart, unless they happen to
+ * own this process (which they may do in the test scenario and
+ * when running Upstart as a non-privileged user).
+ */
+ if (session && session->user && session->user != uid) {

Same comment as for this should be 'session && session->user != uid', and not assume that all users should be allowed to call this method for a non-null session with user==0.

+ if (init_is_upstart ()) {
+ ret = restart_upstart ();
+ }

Please get rid of the init_is_upstart() check (and function); now that restarting is done over dbus, we shouldn't open a separate connection first to check that init is upstart. I think it's reasonable and appropriate that if someone calls 'telinit u' against a non-upstart init, and as a result init does not re-exec itself, telinit should exit non-zero instead of silently failing to take the requested action. Note that this is not an issue for the library upgrade case, because the library packages sensibly do:

 telinit u 2>/dev/null || true

If there is some reason I'm missing that it's important to return 0 in the case of a non-upstart init, please at least don't call upstart_open() twice, but instead save the result so we're not setting up and tearing down the dbus connection twice.

Otherwise, this looks good.

review: Needs Fixing

« Back to merge proposal