Bug#796635: Systemd job
Hi Bryan,
On Thu, 17 Sep 2015 11:45:25 -0400 Bryan Quigley
<bryan.quigley@canonical.com> wrote:
> Here is a debdiff that implements a systemd unit.
>
> (This is the first unit I've written, so review definitely needed)
Thanks for the patch. I agree with your previous comment that this
should maybe be a cronjob instead of boot script, but I get it that
you may want to keep the changes relatively minimal.
Some comments:
> diff -Nru nvi-1.81.6/debian/nvi.init nvi-1.81.6/debian/nvi.init
> --- nvi-1.81.6/debian/nvi.init 1969-12-31 19:00:00.000000000 -0500
> +++ nvi-1.81.6/debian/nvi.init 2015-09-16 22:54:43.000000000 -0400
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +### BEGIN INIT INFO
> +# Provides: nviboot
> +# Required-Start: $remote_fs
> +# Required-Stop:
> +# Default-Start: S
Why did you preserve runlevel S? I don't think this really belongs in
recovery mode.
> +# Default-Stop:
> +# Short-Description: Script to recover nvi edit sessions.
> +### END INIT INFO
> +
> +. /lib/lsb/init-functions
> +
> +case "$1" in
> + start)
> + /usr/share/vi/recover
+1 on moving the logic outside of init script.
> + ;;
> + stop|restart|reload|force-reload)
restart (and force-reload?) should probably re-run the recovery script.
> + ;;
> +esac
> +
> +exit 0
> diff -Nru nvi-1.81.6/debian/nvi.service nvi-1.81.6/debian/nvi.service
> --- nvi-1.81.6/debian/nvi.service 1969-12-31 19:00:00.000000000 -0500
> +++ nvi-1.81.6/debian/nvi.service 2015-09-16 22:54:51.000000000 -0400
> @@ -0,0 +1,9 @@
> +[Unit]
> +Description=To recover nvi edit sessions.
s/To r/R/ ?
> +
> +[Service]
> +Type=oneshot
> +ExecStart=/usr/share/vi/recover
> +
> +[Install]
> +WantedBy=multi-user.target
This will start after basic boot, which is good. I think the init
script should be changed to match this.
Also, the init script Provides: nviboot, so you should provide a link
from nviboot.service to nvi.service in /lib/systemd/system.
> diff -Nru nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch
> --- nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch 1969-12-31 19:00:00.000000000 -0500
> +++ nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch 2015-09-17 11:10:01.000000000 -0400
> @@ -0,0 +1,72 @@
> +Description: Changes recover script so it can be run by init
> +This was mostly just merging code from the previous init script
> +Author: Bryan Quigley <bryan.quigley@canonical.com>
> +
> +---
> +Bug-Debian: https://bugs.debian.org/796635
> +Bug-Ubuntu: https://launchpad.net/bugs/1489939
> +Forwarded: no
> +Last-Update: <2015-09-17>
> +
> +--- nvi-1.81.6.orig/dist/recover.in
> ++++ nvi-1.81.6/dist/recover.in
> +@@ -8,11 +8,18 @@ RECDIR="@vi_cv_path_preserve@"
> + SENDMAIL="@vi_cv_path_sendmail@"
> +
> + echo 'Recovering nvi editor sessions.'
> ++sessions_found=""
> +
> + # Check editor backup files.
> + vibackup=`echo $RECDIR/vi.*`
> + if [ "$vibackup" != "$RECDIR/vi.*" ]; then
> + for i in $vibackup; do
> ++ # Discard symlinks
> ++ if test -L $i ; then
> ++ rm -f $i
> ++ continue
> ++ fi
> ++
> + # Only test files that are readable.
> + if test ! -r $i; then
> + continue
> +@@ -36,6 +43,12 @@ fi
> + virecovery=`echo $RECDIR/recover.*`
> + if [ "$virecovery" != "$RECDIR/recover.*" ]; then
> + for i in $virecovery; do
> ++ # Discard symlinks
> ++ if test -L $i ; then
> ++ rm -f $i
> ++ continue
> ++ fi
> ++
> + # Only test files that are readable.
> + if test ! -r $i; then
> + continue
> +@@ -49,11 +62,21 @@ if [ "$virecovery" != "$RECDIR/recover.*
> + # Delete any recovery files that are zero length, corrupted,
> + # or that have no corresponding backup file. Else send mail
> + # to the user.
> +- recfile=`awk '/^X-vi-recover-path:/{print $2}' < $i`
> +- if test -n "$recfile" -a -s "$recfile"; then
> +- $SENDMAIL -t < $i
> +- else
> +- rm $i
> +- fi
> ++ recfile=`awk '/^X-vi-recover-path:/{print $2}' < $i`
> ++ if test -n "$recfile" -a -s "$recfile"; then
> ++ sessions_found="yes"
> ++ owner=`stat --format='%U' $recfile`
> ++ (su - nobody -s /bin/sh -c "$SENDMAIL $owner < $i" &) </dev/null >/dev/null 2>&0
> ++ else
> ++ rm $i
> ++ fi
> + done
> + fi
> ++
> ++if [ -n "$sessions_found" ] ; then
> ++ echo "done."
> ++else
> ++ echo "none found."
> ++fi
This is a behavior change: previously the recover script would not
print any output. Maybe this should not be printed?
> ++
> ++
> diff -Nru nvi-1.81.6/debian/patches/series nvi-1.81.6/debian/patches/series
> --- nvi-1.81.6/debian/patches/series 2015-09-16 10:16:11.000000000 -0400
> +++ nvi-1.81.6/debian/patches/series 2015-09-17 10:10:36.000000000 -0400
> @@ -25,3 +25,4 @@
> 26trailing_tab_segv.patch
> 27support_C_locale.patch
> 29file_backup.patch
> +30make_recover_script_init_ready.patch
> diff -Nru nvi-1.81.6/debian/rules nvi-1.81.6/debian/rules
> --- nvi-1.81.6/debian/rules 2015-09-16 10:16:11.000000000 -0400
> +++ nvi-1.81.6/debian/rules 2015-09-16 23:06:45.000000000 -0400
> @@ -18,7 +18,7 @@
> endif
>
> %:
> - dh $@
> + dh $@ --with=systemd
>
> override_dh_auto_clean:
> rm -f dist/recover
> @@ -70,4 +70,6 @@
> dh_compress -Xvi.advanced -Xvi.beginner
>
> override_dh_installinit:
> - dh_installinit --init-script=nviboot -- start 70 S .
> + dh_systemd_enable
Why do you invoke dh_systemd_enable manually? the --with=systemd above
should do it.
> + dh_installinit -- start 70 S .
start argument is obsolete now and just invokes `defaults`, so this
override can go.
However, this changes the name of the init script from nviboot to nvi.
This means you need to use dpkg-maintscript-helper to handle the
rename, and remove the old enablement links as well. If you want to do
this you might as well move it out of runlevel S ;)
> + dh_systemd_start --no-start --no-restart-on-upgrade
This one is ok to override, but should be done in override_dh_systemd_start
Saludos,
Reply to: