Code review comment for lp:~ubuntu-core-dev/ubuntu/utopic/sysvinit/unreviewed

> On Mon, May 19, 2014 at 09:06:40AM -0000, Dimitri John Ledkov wrote:
> > @@ -1,3 +1,20 @@
> > +sysvinit (2.88dsf-41ubuntu13) UNRELEASED; urgency=medium
> > +
> > + * Revert previous upload, but keep UPSTARTDIR defined which was still
> > + used in the fallback codepath to specifically prevent configuration
> > + issues in chroots.
>
> What is the reason for this revert, which seems to me to be unrelated to the
> rest of the changes? This seems like a lot of fuss for handling of upstart
> jobs in a chroot, for which the right answer is almost always "install a
> policy-rc.d that blocks all services from starting". Do we have a consensus
> on what the right behavior is here if the host upstart does not support
> chroot sessions, and no policy-rc.d is in place? Should invoke-rc.d error
> out, or should it fall back to starting via the init script, which seem to
> be the two options we're flipping between here?
>

Basically 2.88dsf-41ubuntu11 undefined UPSTARTDIR variable, yet it was still in use

285 if [ ! -e "$UPSTARTDIR/${INITSCRIPTID}.conf" ]; then
286 # If the init script doesn't exist, but the upstart job does, we
287 # defer the error exit; we might be running in a chroot and
288 # policy-rc.d might say not to start the job anyway, in which case
289 # we don't want to exit non-zero.
290 exit 100
291 fi

Thus 2.88dsf-41ubuntu12 revert was uploaded to resolve the regression as soon as possible (it was already in utopic release pocket).

I guess to be more clear this portion of the commit could be:

-UPSTARTDIR=/etc/init/

- && [ -e "$UPSTARTDIR/${INITSCRIPTID}.conf" ]
+ && initctl status ${INITSCRIPTID} 1>/dev/null 2>/dev/null

- if [ ! -e "$UPSTARTDIR/${INITSCRIPTID}.conf" ]; then
+ if [ ! -e "/etc/init/${INITSCRIPTID}.conf" ]; then

Or I can upload it separately.

> > === modified file 'debian/control'
> > --- debian/control 2013-05-17 06:03:10 +0000
> > +++ debian/control 2014-05-19 09:06:30 +0000
> > @@ -43,7 +43,8 @@
> > Depends:
> > ${misc:Depends},
> > sysvinit-utils (>= 2.86.ds1-62),
> > - insserv (>> 1.12.0-10)
> > + insserv (>> 1.12.0-10),
> > + initscripts (>= ${binary:Version})
> > Breaks: initscripts (<< 2.86.ds1-63)
> > Multi-Arch: foreign
> > Description: System-V-like runlevel change mechanism
>
> I don't understand this dependency addition. Maybe this is meant to be
> initscripts (>= 2.88dsf-41ubuntu13), rather than initscripts (>=
> ${binary:Version}) ?
>

Yes, it's meant to be the first version of initscripts that re-introduces these initscripts. If no other uploads happen in the mean time it will become 2.88dsf-41ubuntu13.

> I notice when diffing against initscripts 2.88dsf-41 that there are some
> init scripts that have not been re-added:
>
> -/etc/init.d/bootlogs
> -/etc/init.d/motd
> -/etc/init.d/mtab.sh
> -/etc/init.d/rmnologin
>
> What's different about these?
>

I think they are not used on ubuntu, and are irrelevant under upstart or systemd boot, and not required to enable insserv.

bootlogs -> no dependencies declared in any package in sid, nor ubuntu. Not sure where the equivalent functionality is but my /var/log/dmesg is populated.
motd -> functionality is in mounted-run and it's backgrounded, packages on ubuntu should instead integrate with /etc/update-motd.d (only user is mythtv-status)
mtab.sh -> initscript not present in sid, only reference is dummy job in upstart
rmnologin -> only user is sudo with X-Start-Before.

I don't see a point in introducing those + dummy systemd units symlinking to /dev/null + dummy upstart jobs.

> > === modified file 'debian/initscripts.postinst'
> > --- debian/initscripts.postinst 2013-05-17 06:03:10 +0000
> > +++ debian/initscripts.postinst 2014-05-19 09:06:30 +0000
> > @@ -140,6 +140,39 @@
> > #
> > # Links in runlevel S
> > #
> > +if [ -x /etc/init.d/mountkernfs.sh ]; then
> > +update-rc.d mountkernfs.sh start 02 S . >/dev/null || exit $?
> > +fi
>
> <snip>
>
> Per
> <https://bugs.launchpad.net/ubuntu/+source/ifupdown/+bug/1312836/comments/7>,
> we don't want to reintroduce the calls to update-rc.d until /after/ insserv
> has been re-enabled and fully vetted (i.e., step 4 - drop the debhelper
> change that causes update-rc.d to be skipped... this also applies to
> sysvinit, even though sysvinit doesn't use the debhelper implementation.)
>

However sysvinit & these scripts are special. Even with update-rc.d, they do nothing =) under systemd they are symlinked to /dev/null & under upstart they simply start compat jobs which have no body / processes. Thus there is no harm in calling update-rc.d for these jobs. And my understanding was that they are required to have been enabled for insserv conversion to work. If they simply need to be in /etc/init.d/ that's fine, but then we'd need another upload to add update-rc.d calls?

> Also, please note that this postinst still contains a delta to call
> update-rc.d *remove* immediately above your restored calls, which ought to
> be dropped at the same time as restoring the code to add the links.
>
> And the code in debian/initscripts.preinst which deletes these restored
> initscripts definitely wants to be removed.
>

Ok. I didn't consider dropping it, as it is properly version guarded and adds more to the review diff. For sanity sake I'll drop them.

> > === added file 'debian/src/initscripts/etc/init.d/bootmisc.sh'
> > --- debian/src/initscripts/etc/init.d/bootmisc.sh 1970-01-01 00:00:00
> +0000
> > +++ debian/src/initscripts/etc/init.d/bootmisc.sh 2014-05-19 09:06:30
> +0000
> <snip>
> > +# NB! on ubuntu this file is usually not used at all, as it's
> > +# redundant under upstart or systemd. This file is purely introduced
> > +# to enable insserv / systemd init.d scripts activation
> > +. /lib/lsb/init-functions
>
> IMHO the benefit of this expository comment is outweighed by the detriment
> of having the extra delta in these scripts; but that's just my opinion.
>

Ok. But I had to add diffs in a couple of scripts to source "./lib/lsb/init-functions" where it was not already sourced and that would imho warrant some comment, as otherwise it's not obvious that it's sourced for the side-effect reasons.

I'll drop comments from scripts that i didn't need to modify.

> > === modified file 'debian/sysv-rc.postinst'
> > --- debian/sysv-rc.postinst 2013-05-17 06:03:10 +0000
> > +++ debian/sysv-rc.postinst 2014-05-19 09:06:30 +0000
> > @@ -7,7 +7,7 @@
> > logfile="$logdir/run-$now.log"
> >
> > # Make sure insserv is in path
> > -PATH=/sbin:$PATH
> > +PATH=/sbin:/usr/lib/insserv/:$PATH
>
> I don't understand why this is needed - is this a bug in Debian that's been
> overlooked because the insserv activation has already been done there, so
> this postinst code is no longer relevant in Debian?
>

Rather Ubuntu didn't yet follow Debian in making insserv to be available on the path. insserv (src:insserv) in Debian is in /sbin, if we can move our insserv to be available on normal path, then we wouldn't need above hunk.

> > # Based on code from dash postinst
> > check_divert() {
> > @@ -41,7 +41,7 @@
> > }
> >
> > legacy_bootordering() {
> > - for f in /etc/rc0.d/S* ; do
> > + for f in /etc/rc0.d/S* /etc/rc6.d/S* ; do
> > if [ -f $f ] ; then
> > return 0
> > fi
>
> This corresponds to the commit / changelog entry:
>
> adjust legacy_boot check to examine both rc0.d and rc6.d and the
> stamp file
>
> But why is this needed? What is your case where /etc/rc0.d/S* wouldn't
> match any files, but /etc/rc6.d/S* would or /etc/init.d/.legacy-bootordering
> would be present?
>
> If there's not a specific reason for this construction, I think it would be
> better to use the Debian implementation and further reduce our delta.

I found this check to be incomplete. I had rc0 conversion that did succeed, yet rc6 did not. Package configuration failed, and later attempts to configure did not trigger the upgrade any more. Thus I'd would want to extend the legacy_booting check to be:
- for f in /etc/rc0.d/S* ; do
+ for f in /etc/rc0.d/S* /etc/rc6.d/S* /etc/init.d/.legacy-bootordering; do