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

On Wed, May 21, 2014 at 02:26:50PM -0000, Dimitri John Ledkov wrote:
> 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.

Fine with me for it to be included, now that I understand.

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

Ok, please don't forget to change it. :)

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

<snip>

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

Makes sense, thanks.

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

That's only true if you have a versioned dependency on the version of
upstart that introduces the init-functions.d hook, which you do not.

> 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?

Ok, I think you're right that they need to be enabled with update-rc.d in
order for insserv to DTRT.

I guess there are no other packages that have had init scripts dropped in
Ubuntu that need re-enabled before we turn on insserv?

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

Right, understood.

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

On the contrary, insserv is absent from the path in Ubuntu because of an
Ubuntu-specific change to *prevent* it from being called on systems where
it's not supported; but in the meantime, the Debian maintainers have decided
this is a good idea and in experimental, a patch has been uploaded to move
insserv *off* the path:

insserv (1.16.0-5) experimental; urgency=low

  * Add 200_hide_insserv_on_ubuntu.patch to make sure insserv isn't in
    the default PATH, and break sysv-rc (<< 2.88dsf-53) to ensure a new version
    of sysvinit is used with this version of the insserv package.
  * Add autopkgtest self test code using the test suite executed at build
    time.

 -- Petter Reinholdtsen <email address hidden> Thu, 17 Apr 2014 18:35:27 +0200

(but, well, it didn't actually work. Oops.)

> 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 the above, we should expect insserv to not be on the default path
in the future.

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

Ok, makes sense.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>