[arch-projects] [initscripts] [PATCH 20/20] Catch all errors within status blocks

Mon Jul 11 12:38:24 EDT 2011

Whitespace changes are introduced because of out status block 
indentation rule.

I always attempted to make things more readable and elegant. First i 
changed five line if/stat_{done,fail} clauses into one liners using '&&' 
and '||'. Now i change these into two lines without any conditional
constructs because whether stat_done or stat_fail is actually done no 
longer depends on the last command only, but on all commands within the 
status block. I thing the code would look better then ever before with 
this now.

As said in the commit, it allows to catch errors caused by missing utmp 
group. It would also allow to catch errors caused by missing API-FS 
mountpoints or missing binaries called somewhere in the middle of a 
status block etc.

I think we should avoid to say '[DONE]' in cases where actually only the 
last action succeeded in a multi actions block like 'Removing Leftover 
Files'.

To get more fine grained error messages, we would just need to remove 
all std_err redirections where ever possible as done for cleaning /tmp/ 
here (by using correct patterns).

In fact i think this makes the code more readable and avoiding hidden 
errors would make troubleshooting (a system where initscripts is 
installed) much easier. Actually all kinds of subshells and also 
function bodies aren't involved here as long as we don't use 'set -E'.

trap 'echo err' ERR
cat <( false )
# <<< you see nothing here unless you have done 'set -E'

Yes, i also like TOFU (sometimes).

-- 
Kurt

Dave Reisner, 2011-07-10 21:30:
> You've got large quantities of non-functional changes in here
> (whitespace churn), mixed in with reverts to your previous attempts at
> turning everything into a single line. Can we pick a direction and stick
> with it? This all seems like a lot of cleverness to accomplish...
> well...  I'm not sure. What exactly _does_ this accomplish that we
> didn't previously have? If we want to be more fine grained about error
> checking as your rationale advertises, then let's do that so we can
> report a more detailed error than "[FAIL]".
>
> I'm sure I've mentioned this before, but we value simplicity and
> readability in the initscripts. This is crucial code that cannot go foul
> because of some bash side effect we haven't considered. I'd rather be a
> little more verbose and make this easier to troubleshoot than to have to
> worry about write errors on process substitutions triggering magical ERR
> trap causing an echo somewhere to claim FAIL.
>
> Yes, I know I'm top posting.
>
> d
>
> On Sun, Jul 10, 2011 at 08:58:39PM +0200, Kurt J. Bosch wrote:
>> Rationale:
>> Instead of ignoring any errors of all commands but the last within a status
>> block it is much better to use trap to catch them and report 'FAIL'. One
>> might for example miss the utmp group which is needed to propperly install
>> /var/run/utmp.
>> This also makes the code a bit more simple and readable.
>>
>> Note:
>> We enable this explicitely (by calling stat_err_trap) because most daemon
>> scriptlets don't work well with this by now. Moreover we don't use 'set -E'
>> to avoid breaking/complicating [custom/hook] functions.
>> ---
>>   functions   |   10 ++++++++++
>>   netfs       |   12 ++++--------
>>   rc.shutdown |    2 ++
>>   rc.single   |    2 ++
>>   rc.sysinit  |   59 +++++++++++++++++++++++++++++++++--------------------------
>>   5 files changed, 51 insertions(+), 34 deletions(-)
>>
>> diff --git a/functions b/functions
>> index d1d2947..ac5394c 100644
>> --- a/functions
>> +++ b/functions
>> @@ -125,11 +125,17 @@ stat_bkgd() {
>>   	printf "   ${C_OTHER}[${C_BKGD}BKGD${C_OTHER}]${C_CLEAR} "
>>   }
>>
>> +# Allow to catch any errors and get the exit-code of the last failing command
>> +stat_err_trap() {
>> +	trap 'STAT_ERR=$?' ERR
>> +}
>> +
>>   stat_busy() {
>>   	printf "${C_OTHER}${PREFIX_REG} ${C_MAIN}${1}${C_CLEAR} "
>>   	printf "${SAVE_POSITION}"
>>   	deltext
>>   	printf "   ${C_OTHER}[${C_BUSY}BUSY${C_OTHER}]${C_CLEAR} "
>> +	STAT_ERR=0
>>   }
>>
>>   stat_append() {
>> @@ -139,6 +145,10 @@ stat_append() {
>>   }
>>
>>   stat_done() {
>> +	if (( STAT_ERR )); then
>> +		stat_fail
>> +		return $STAT_ERR
>> +	fi
>>   	deltext
>>   	printf "   ${C_OTHER}[${C_DONE}DONE${C_OTHER}]${C_CLEAR} \n"
>>   }
>> diff --git a/netfs b/netfs
>> index ea7e4eb..4dc91ef 100755
>> --- a/netfs
>> +++ b/netfs
>> @@ -4,24 +4,20 @@
>>   . /etc/rc.conf
>>   . /etc/rc.d/functions
>>
>> +stat_err_trap
>> +
>>   case "$1" in
>>   	start)
>>   		stat_busy "Mounting Network Filesystems"
>>   		mount -a -t "$NETFS"
>> -		rc=$?
>>   		mount -a -O _netdev
>> -		(( rc || $? ))&&  stat_die
>> -		add_daemon netfs
>> -		stat_done
>> +		stat_done&&  add_daemon netfs || exit 1
>>   		;;
>>   	stop)
>>   		stat_busy "Unmounting Network Filesystems"
>>   		umount -a -O _netdev
>> -		rc=$?
>>   		umount -a -t "$NETFS"
>> -		(( rc || $? ))&&  stat_die
>> -		rm_daemon netfs
>> -		stat_done
>> +		stat_done&&  rm_daemon netfs || exit 1
>>   		;;
>>   	restart)
>>   		$0 stop
>> diff --git a/rc.shutdown b/rc.shutdown
>> index ed87eec..db4f12b 100755
>> --- a/rc.shutdown
>> +++ b/rc.shutdown
>> @@ -6,6 +6,8 @@
>>   . /etc/rc.conf
>>   . /etc/rc.d/functions
>>
>> +stat_err_trap
>> +
>>   run_hook shutdown_start
>>
>>   # avoid staircase effect
>> diff --git a/rc.single b/rc.single
>> index 7a87c72..b5c6e66 100755
>> --- a/rc.single
>> +++ b/rc.single
>> @@ -6,6 +6,8 @@
>>   . /etc/rc.conf
>>   . /etc/rc.d/functions
>>
>> +stat_err_trap
>> +
>>   run_hook single_start
>>
>>   if [[ $PREVLEVEL != N ]]; then
>> diff --git a/rc.sysinit b/rc.sysinit
>> index 97c16c8..be8eb05 100755
>> --- a/rc.sysinit
>> +++ b/rc.sysinit
>> @@ -6,6 +6,8 @@
>>   . /etc/rc.conf
>>   . /etc/rc.d/functions
>>
>> +stat_err_trap
>> +
>>   echo " "
>>   printhl "Arch Linux\n"
>>   printhl "${C_H2}http://www.archlinux.org"
>> @@ -14,17 +16,19 @@ printsep
>>   run_hook sysinit_start
>>
>>   # mount /proc, /sys, /run, /dev, /run/lock, /dev/pts, /dev/shm (the api filesystems)
>> -mountpoint -q /proc    || mount -n -t proc proc /proc -o nosuid,noexec,nodev
>> -mountpoint -q /sys     || mount -n -t sysfs sys /sys -o nosuid,noexec,nodev
>> -mountpoint -q /run     || mount -n -t tmpfs run /run -o mode=0755,size=10M,nosuid,nodev
>> -mountpoint -q /dev     || mount -n -t devtmpfs udev /dev -o mode=0755,size=10M,nosuid&>/dev/null \
>> -	|| mount -n -t tmpfs udev /dev -o mode=0755,size=10M,nosuid
>> -mkdir -p -m 1777 /run/lock
>> -mkdir -p /dev/{pts,shm}
>> -mountpoint -q /dev/pts || mount -n /dev/pts&>/dev/null \
>> -	|| mount -n -t devpts devpts /dev/pts -o mode=0620,gid=5,nosuid,noexec
>> -mountpoint -q /dev/shm || mount -n /dev/shm&>/dev/null \
>> -	|| mount -n -t tmpfs shm /dev/shm -o mode=1777,nosuid,nodev
>> +stat_busy "Mounting API Filesystems"
>> +	mountpoint -q /proc    || mount -n -t proc proc /proc -o nosuid,noexec,nodev
>> +	mountpoint -q /sys     || mount -n -t sysfs sys /sys -o nosuid,noexec,nodev
>> +	mountpoint -q /run     || mount -n -t tmpfs run /run -o mode=0755,size=10M,nosuid,nodev
>> +	mountpoint -q /dev     || mount -n -t devtmpfs udev /dev -o mode=0755,size=10M,nosuid&>/dev/null \
>> +		|| mount -n -t tmpfs udev /dev -o mode=0755,size=10M,nosuid
>> +	mkdir -p -m 1777 /run/lock
>> +	mkdir -p /dev/{pts,shm}
>> +	mountpoint -q /dev/pts || mount -n /dev/pts&>/dev/null \
>> +		|| mount -n -t devpts devpts /dev/pts -o mode=0620,gid=5,nosuid,noexec
>> +	mountpoint -q /dev/shm || mount -n /dev/shm&>/dev/null \
>> +		|| mount -n -t tmpfs shm /dev/shm -o mode=1777,nosuid,nodev
>> +stat_done
>>
>>   # remount root ro to allow for fsck later on, we remount now to
>>   # make sure nothing can open files rw on root which would block a remount
>> @@ -46,7 +50,7 @@ esac
>>   if [[ $HWCLOCK_PARAMS ]]; then
>>   	stat_busy "Adjusting system time and setting kernel timezone"
>>   		# enable rtc access
>> -		modprobe -q -a rtc-cmos rtc genrtc
>> +		modprobe -q -a rtc-cmos rtc genrtc || :
>>   		# If devtmpfs is used, the required RTC device already exists now
>>   		# Otherwise, create whatever device is available
>>   		if ! [[ -c /dev/rtc || -c /dev/rtc0 ]]; then
>> @@ -65,7 +69,8 @@ if [[ $HWCLOCK_PARAMS ]]; then
>>   		# is used. If HARDWARECLOCK is not set in rc.conf, the value in
>>   		# /var/lib/hwclock/adjfile is used (in this case /var can not be a separate
>>   		# partition).
>> -	TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS&&  stat_done || stat_fail
>> +		TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS
>> +	stat_done
>>   fi
>>
>>   # Start/trigger UDev, load MODULES and settle UDev
>> @@ -89,7 +94,7 @@ activate_vgs
>>   # Set up non-root encrypted partition mappings
>>   if [[ -f /etc/crypttab&&  $CS ]]&&  grep -q ^[^#] /etc/crypttab; then
>>   	stat_busy "Unlocking encrypted volumes:"
>> -		modprobe -q dm-crypt 2>/dev/null
>> +		modprobe -q dm-crypt 2>/dev/null || :
>>   		do_unlock() {
>>   			# $1 = requested name
>>   			# $2 = source device
>> @@ -167,8 +172,9 @@ if [[ -f /etc/crypttab&&  $CS ]]&&  grep -q ^[^#] /etc/crypttab; then
>>   			fi
>>   			return $failed
>>   		}
>> -	crypto_unlocked=0
>> -	read_crypttab do_unlock&&  stat_done || stat_fail
>> +		crypto_unlocked=0
>> +		read_crypttab do_unlock
>> +	stat_done
>>   	# Maybe someone has LVM on an encrypted block device
>>   	(( crypto_unlocked == 1 ))&&  activate_vgs
>>   fi
>> @@ -177,14 +183,13 @@ fi
>>   [[ -f /forcefsck || " $(<  /proc/cmdline) " =~ [[:blank:]]forcefsck[[:blank:]] ]]&&  FORCEFSCK="-- -f"
>>   declare -r FORCEFSCK
>>   run_hook sysinit_prefsck
>> +fsckret=0
>>   if [[ -x $(type -P fsck) ]]; then
>>   	stat_busy "Checking Filesystems"
>> -		fsck_all>|${FSCK_OUT:-/dev/stdout} 2>|${FSCK_ERR:-/dev/stdout}
>> -	declare -r fsckret=$?
>> +		fsck_all>|${FSCK_OUT:-/dev/stdout} 2>|${FSCK_ERR:-/dev/stdout} || fsckret=$?
>>   	(( fsckret<= 1 ))&&  stat_done || stat_fail
>> -else
>> -	declare -r fsckret=0
>>   fi
>> +declare -r fsckret
>>   run_hook sysinit_postfsck
>>
>>   # Single-user login and/or automatic reboot if needed
>> @@ -201,7 +206,7 @@ if [[ ! -L /etc/mtab ]]; then
>>   		else
>>   			cat /proc/mounts>| /etc/mtab
>>   		fi
>> -	(( $? == 0 ))&&  stat_done || stat_fail
>> +	stat_done
>>   fi
>>
>>   # now mount all the local filesystems
>> @@ -227,7 +232,7 @@ RANDOM_SEED=/var/lib/misc/random-seed
>>   		cp $RANDOM_SEED /dev/urandom
>>
>>   stat_busy "Removing Leftover Files"
>> -	rm -rf /etc/{nologin,shutdownpid} /forcefsck /tmp/* /tmp/.* /var/run/daemons&>/dev/null
>> +	rm -rf /etc/{nologin,shutdownpid} /forcefsck /tmp/* /tmp/.[^.]* /tmp/..?* /var/run/daemons
>>   	[[ -d /var/lock&&  $(stat -f -c %T -L /var/lock) != tmpfs ]]&&  rm -rf /var/lock/*
>>   	if [[ -d /var/run&&  $(stat -f -c %T -L /var/run) != tmpfs ]]; then
>>   		find /var/run/ \! -type d -delete
>> @@ -240,13 +245,15 @@ stat_done
>>
>>   if [[ $HOSTNAME ]]; then
>>   	stat_busy "Setting Hostname: $HOSTNAME"
>> -	echo "$HOSTNAME">| /proc/sys/kernel/hostname&&  stat_done || stat_fail
>> +		echo "$HOSTNAME">| /proc/sys/kernel/hostname
>> +	stat_done
>>   fi
>>
>>   # Flush old locale settings and set user defined locale
>>   stat_busy "Setting Locale: ${LOCALE:=en_US}"
>> -	echo "export LANG=$LOCALE">  /etc/profile.d/locale.sh&&
>> -chmod 0755 /etc/profile.d/locale.sh&&  stat_done || stat_fail
>> +	echo "export LANG=$LOCALE">  /etc/profile.d/locale.sh
>> +	chmod 0755 /etc/profile.d/locale.sh
>> +stat_done
>>
>>   if [[ ${LOCALE,,} =~ utf ]]; then
>>   	stat_busy "Setting Consoles to UTF-8 mode"
>> @@ -282,7 +289,7 @@ stat_busy "Saving dmesg Log"
>>   	else
>>   		install -Tm 0644<( dmesg ) /var/log/dmesg.log
>>   	fi
>> -(( $? == 0 ))&&  stat_done || stat_fail
>> +stat_done
>>
>>   run_hook sysinit_end
>>
>> --
>> 1.7.1
>>