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

Revision history for this message
James Hunt (jamesodhunt) 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 https://code.launchpad.net/~jamesodhunt/upstart/bug-1079715/+merge/135388: 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.

Fixed.

>
> + 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.

The initial init_is_upstart() check would have given us the opportunity to display a cleaner error message in the non-Upstart init scenario without more invasive code changes (something along the lines of "telinit: ERROR - init is not Upstart"). What currently happens if /sbin/init is not Upstart is this:

    # telinit u
    telinit: Failed to connect to socket /var/run/dbus/system_bus_socket: No such file or directory
    # echo $?
    1

It's not particularly pretty, but we don't anticipate this scenario happening much and of course it can be quietened down by redirecting stderr if required.

« Back to merge proposal