[arch-projects] [initscripts] [PATCH 00/12] Various minor fixes for consistency mainly

newer
[arch-projects] [MKINITCPIO][PATCH...

Kurt J. Bosch

25 Jun 2011 25 Jun '11

12:15 p.m.

Kurt J. Bosch (12): Add some more comments mtab, dmesg: show status crypt, lvm: move stat_busy to the top functions: fix indentation fsck_all, kill_everything: Clean up run_hook positions ck_depends, ck_pidfile, do_unlock, kill_everything: add missing local statements Move check on executable fsck into fsck_all() for consistency activate_vgs, fsck_reboot: fix initial return statements for consistecy udevd_modprobe: Get rid of simple if-constuct by using '&&' Get rid of more simple if-constucts by using '&&' and '||' Simplify LC_* unsetting Move export PATH into functions functions | 112 ++++++++++++++++++++++++++++------------------------------ rc.shutdown | 2 - rc.sysinit | 72 +++++++++++++++++--------------------- 3 files changed, 86 insertions(+), 100 deletions(-)

Show replies by date

Kurt J. Bosch

25 Jun 25 Jun

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 01/12] Add some more comments

--- functions | 4 +++- rc.sysinit | 3 +++ 2 files changed, 6 insertions(+), 1 deletions(-) diff --git a/functions b/functions index b64f78b..6666acf 100644 --- a/functions +++ b/functions @@ -368,6 +368,7 @@ read_crypttab() { NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuseblk,noglusterfs,nodavfs" +# Check local filesystems fsck_all() { stat_busy "Checking Filesystems" FSCK_OUT=/dev/stdout @@ -387,6 +388,7 @@ fsck_all() { return $fsckret } +# Single-user login and/or automatic reboot after fsck if needed fsck_reboot() { # $1 = exit code returned by fsck # Ignore conditions 'FS errors corrected' and 'Cancelled by the user' @@ -496,7 +498,7 @@ if (( RC_FUNCTIONS_HOOK_FUNCS_DEFINED != 1 )); then declare -r RC_FUNCTIONS_HOOK_FUNCS_DEFINED=1 fi -# Function for setting console font if required +# Set console font set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" diff --git a/rc.sysinit b/rc.sysinit index a2c775f..50a8faf 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -90,6 +90,7 @@ udevd_modprobe sysinit [[ $USEBTRFS = [Yy][Ee][Ss] && -x $(type -P btrfs) ]] && status "Activating BTRFS volumes" btrfs device scan +# Activate LVM2 groups if any activate_vgs # Set up non-root encrypted partition mappings @@ -186,7 +187,9 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then fi if [[ -x $(type -P fsck) ]]; then + # Check filesystems fsck_all + # Single-user login and/or automatic reboot if needed fsck_reboot $? fi -- 1.7.1

Kurt J. Bosch

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 02/12] mtab, dmesg: show status

--- rc.sysinit | 28 ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-) diff --git a/rc.sysinit b/rc.sysinit index 50a8faf..225790d 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -197,12 +197,14 @@ status "Remounting Root Read/Write" \ mount -n -o remount,rw / # don't touch /etc/mtab if it is a symlink to /proc/self/mounts -if [[ -L /etc/mtab ]]; then - : -elif [[ -x $(type -P findmnt) && -e /proc/self/mountinfo ]]; then - findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS >| /etc/mtab -else - cat /proc/mounts >| /etc/mtab +if [[ ! -L /etc/mtab ]]; then + stat_busy "Creating mtab" + if [[ -x $(type -P findmnt) && -e /proc/self/mountinfo ]]; then + findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS >| /etc/mtab + else + cat /proc/mounts >| /etc/mtab + fi + if (( $? == 0 )); then stat_done; else stat_fail; fi fi # now mount all the local filesystems @@ -271,12 +273,14 @@ fi # Set console font if required set_consolefont -if [[ -e /proc/sys/kernel/dmesg_restrict ]] && - (( $(< /proc/sys/kernel/dmesg_restrict) == 1 )); then - install -Tm 0600 <( dmesg ) /var/log/dmesg.log -else - install -Tm 0644 <( dmesg ) /var/log/dmesg.log -fi +stat_busy "Saving dmesg Log" + if [[ -e /proc/sys/kernel/dmesg_restrict ]] && + (( $(< /proc/sys/kernel/dmesg_restrict) == 1 )); then + install -Tm 0600 <( dmesg ) /var/log/dmesg.log + else + install -Tm 0644 <( dmesg ) /var/log/dmesg.log + fi +if (( $? == 0 )); then stat_done; else stat_fail; fi run_hook sysinit_end -- 1.7.1

Dave Reisner

3:41 p.m.

New subject: [arch-projects] [initscripts] [PATCH 02/12] mtab, dmesg: show status

On Sat, Jun 25, 2011 at 12:15:54PM +0200, Kurt J. Bosch wrote:

...

--- rc.sysinit | 28 ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/rc.sysinit b/rc.sysinit index 50a8faf..225790d 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -197,12 +197,14 @@ status "Remounting Root Read/Write" \ mount -n -o remount,rw /

# don't touch /etc/mtab if it is a symlink to /proc/self/mounts -if [[ -L /etc/mtab ]]; then - : -elif [[ -x $(type -P findmnt) && -e /proc/self/mountinfo ]]; then - findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS >| /etc/mtab -else - cat /proc/mounts >| /etc/mtab +if [[ ! -L /etc/mtab ]]; then + stat_busy "Creating mtab" + if [[ -x $(type -P findmnt) && -e /proc/self/mountinfo ]]; then + findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS >| /etc/mtab + else + cat /proc/mounts >| /etc/mtab + fi + if (( $? == 0 )); then stat_done; else stat_fail; fi

Can we stay away from these one line if statements? We don't use them anywhere else. Otherwise, ACK. d

...

fi

# now mount all the local filesystems @@ -271,12 +273,14 @@ fi # Set console font if required set_consolefont

-if [[ -e /proc/sys/kernel/dmesg_restrict ]] && - (( $(< /proc/sys/kernel/dmesg_restrict) == 1 )); then - install -Tm 0600 <( dmesg ) /var/log/dmesg.log -else - install -Tm 0644 <( dmesg ) /var/log/dmesg.log -fi +stat_busy "Saving dmesg Log" + if [[ -e /proc/sys/kernel/dmesg_restrict ]] && + (( $(< /proc/sys/kernel/dmesg_restrict) == 1 )); then + install -Tm 0600 <( dmesg ) /var/log/dmesg.log + else + install -Tm 0644 <( dmesg ) /var/log/dmesg.log + fi +if (( $? == 0 )); then stat_done; else stat_fail; fi

run_hook sysinit_end

-- 1.7.1

Kurt J. Bosch

3:45 p.m.

New subject: [arch-projects] [initscripts] [PATCH 02/12] mtab, dmesg: show status

Dave Reisner, 2011-06-25 15:41:

...

On Sat, Jun 25, 2011 at 12:15:54PM +0200, Kurt J. Bosch wrote:

...
--- rc.sysinit | 28 ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/rc.sysinit b/rc.sysinit index 50a8faf..225790d 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -197,12 +197,14 @@ status "Remounting Root Read/Write" \ mount -n -o remount,rw /

# don't touch /etc/mtab if it is a symlink to /proc/self/mounts -if [[ -L /etc/mtab ]]; then - : -elif [[ -x $(type -P findmnt)&& -e /proc/self/mountinfo ]]; then - findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS>| /etc/mtab -else - cat /proc/mounts>| /etc/mtab +if [[ ! -L /etc/mtab ]]; then + stat_busy "Creating mtab" + if [[ -x $(type -P findmnt)&& -e /proc/self/mountinfo ]]; then + findmnt -rnu -o SOURCE,TARGET,FSTYPE,OPTIONS>| /etc/mtab + else + cat /proc/mounts>| /etc/mtab + fi + if (( $? == 0 )); then stat_done; else stat_fail; fi

Can we stay away from these one line if statements? We don't use them anywhere else. Otherwise, ACK.

These are changed in a later patch into (( $? == 0 )) && stat_done || stat_fail I guess that looks much better, doesn't it?

Kurt J. Bosch

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 03/12] crypt, lvm: move stat_busy to the top

--- functions | 2 +- rc.sysinit | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/functions b/functions index 6666acf..61cf3dd 100644 --- a/functions +++ b/functions @@ -330,9 +330,9 @@ udevd_modprobe() { activate_vgs() { [[ $USELVM = [yY][eE][sS] && -x $(type -P lvm) && -d /sys/block ]] || return + stat_busy "Activating LVM2 groups" # Kernel 2.6.x, LVM2 groups /sbin/modprobe -q dm-mod 2>/dev/null - stat_busy "Activating LVM2 groups" if /sbin/vgchange --sysinit -a y >/dev/null; then stat_done else diff --git a/rc.sysinit b/rc.sysinit index 225790d..5edffe1 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -95,8 +95,8 @@ activate_vgs # Set up non-root encrypted partition mappings if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then - modprobe -q dm-crypt 2>/dev/null stat_busy "Unlocking encrypted volumes:" + modprobe -q dm-crypt 2>/dev/null do_unlock() { # $1 = requested name # $2 = source device -- 1.7.1

Kurt J. Bosch

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

--- functions | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-) diff --git a/functions b/functions index 61cf3dd..9c881c1 100644 --- a/functions +++ b/functions @@ -285,17 +285,17 @@ kill_everything() { # Terminate all processes stat_busy "Sending SIGTERM To Processes" run_hook "$1_prekillall" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args &>/dev/null - /bin/sleep 5 + local pid k5args="" + for pid in ${omit_pids[@]}; do + k5args+=" -o $pid" + done + /sbin/killall5 -15 $k5args &>/dev/null + /bin/sleep 5 stat_done stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args &>/dev/null - /bin/sleep 1 + /sbin/killall5 -9 $k5args &>/dev/null + /bin/sleep 1 stat_done run_hook "$1_postkillall" @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { stat_busy "Checking Filesystems" - FSCK_OUT=/dev/stdout - FSCK_ERR=/dev/stdout - FSCK_FD= - FORCEFSCK= - [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" - run_hook sysinit_prefsck - fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR + FSCK_OUT=/dev/stdout + FSCK_ERR=/dev/stdout + FSCK_FD= + FORCEFSCK= + [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" + run_hook sysinit_prefsck + fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR local fsckret=$? if (( fsckret > 1 )); then stat_fail @@ -426,9 +426,9 @@ fsck_reboot() { mount_all() { stat_busy "Mounting Local Filesystems" - run_hook sysinit_premount - mount -a -t $NETFS -O no_netdev - run_hook sysinit_postmount + run_hook sysinit_premount + mount -a -t $NETFS -O no_netdev + run_hook sysinit_postmount stat_done } @@ -502,13 +502,13 @@ fi set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" - #CONSOLEMAP in UTF-8 shouldn't be used - [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" - local i - for i in /dev/tty[0-9]*; do - /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ - $CONSOLEFONT -C ${i} &>/dev/null - done + #CONSOLEMAP in UTF-8 shouldn't be used + [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" + local i + for i in /dev/tty[0-9]*; do + /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ + $CONSOLEFONT -C ${i} &>/dev/null + done if (( $? )); then stat_fail elif [[ $CONSOLEMAP ]]; then -- 1.7.1

Dave Reisner

8:11 p.m.

New subject: [arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

On Sat, Jun 25, 2011 at 12:15:56PM +0200, Kurt J. Bosch wrote:

...

--- functions | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/functions b/functions index 61cf3dd..9c881c1 100644 --- a/functions +++ b/functions @@ -285,17 +285,17 @@ kill_everything() { # Terminate all processes stat_busy "Sending SIGTERM To Processes" run_hook "$1_prekillall" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args &>/dev/null - /bin/sleep 5 + local pid k5args="" + for pid in ${omit_pids[@]}; do + k5args+=" -o $pid" + done

I'd rather just see this whole chunk of code go away. Building the extra variable is moot when you can just use a PE: killall 5 -15 ${omit_pids[@]/#/-o } d

...

+ /sbin/killall5 -15 $k5args &>/dev/null + /bin/sleep 5 stat_done

stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args &>/dev/null - /bin/sleep 1 + /sbin/killall5 -9 $k5args &>/dev/null + /bin/sleep 1 stat_done

run_hook "$1_postkillall" @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { stat_busy "Checking Filesystems" - FSCK_OUT=/dev/stdout - FSCK_ERR=/dev/stdout - FSCK_FD= - FORCEFSCK= - [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" - run_hook sysinit_prefsck - fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR + FSCK_OUT=/dev/stdout + FSCK_ERR=/dev/stdout + FSCK_FD= + FORCEFSCK= + [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" + run_hook sysinit_prefsck + fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR local fsckret=$? if (( fsckret > 1 )); then stat_fail @@ -426,9 +426,9 @@ fsck_reboot() {

mount_all() { stat_busy "Mounting Local Filesystems" - run_hook sysinit_premount - mount -a -t $NETFS -O no_netdev - run_hook sysinit_postmount + run_hook sysinit_premount + mount -a -t $NETFS -O no_netdev + run_hook sysinit_postmount stat_done }

@@ -502,13 +502,13 @@ fi set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" - #CONSOLEMAP in UTF-8 shouldn't be used - [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" - local i - for i in /dev/tty[0-9]*; do - /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ - $CONSOLEFONT -C ${i} &>/dev/null - done + #CONSOLEMAP in UTF-8 shouldn't be used + [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" + local i + for i in /dev/tty[0-9]*; do + /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ + $CONSOLEFONT -C ${i} &>/dev/null + done if (( $? )); then stat_fail elif [[ $CONSOLEMAP ]]; then -- 1.7.1

I never really understood why we indented like this. Since we seem to be split 50/50, I'd argue that it might be better to just separate the code with whitespace, e.g. # more code above here... stat_busy foo bar baz stat_done # more code below here... This way, we don't have to worry about what happens when there could be a stat_fail somewhere in between that would make us have to wonder wtf to do with indenting. d

Tom Gundersen

9:10 p.m.

New subject: [arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

On Jun 25, 2011, at 20:11, Dave Reisner wrote:

...

On Sat, Jun 25, 2011 at 12:15:56PM +0200, Kurt J. Bosch wrote:

...
--- functions | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/functions b/functions index 61cf3dd..9c881c1 100644 --- a/functions +++ b/functions @@ -285,17 +285,17 @@ kill_everything() { # Terminate all processes stat_busy "Sending SIGTERM To Processes" run_hook "$1_prekillall" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args &>/dev/null - /bin/sleep 5 + local pid k5args="" + for pid in ${omit_pids[@]}; do + k5args+=" -o $pid" + done

I'd rather just see this whole chunk of code go away. Building the extra variable is moot when you can just use a PE:

killall 5 -15 ${omit_pids[@]/#/-o }

d

...
+ /sbin/killall5 -15 $k5args &>/dev/null + /bin/sleep 5 stat_done

stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args &>/dev/null - /bin/sleep 1 + /sbin/killall5 -9 $k5args &>/dev/null + /bin/sleep 1 stat_done

run_hook "$1_postkillall" @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { stat_busy "Checking Filesystems" - FSCK_OUT=/dev/stdout - FSCK_ERR=/dev/stdout - FSCK_FD= - FORCEFSCK= - [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" - run_hook sysinit_prefsck - fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR + FSCK_OUT=/dev/stdout + FSCK_ERR=/dev/stdout + FSCK_FD= + FORCEFSCK= + [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" + run_hook sysinit_prefsck + fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR local fsckret=$? if (( fsckret > 1 )); then stat_fail @@ -426,9 +426,9 @@ fsck_reboot() {

mount_all() { stat_busy "Mounting Local Filesystems" - run_hook sysinit_premount - mount -a -t $NETFS -O no_netdev - run_hook sysinit_postmount + run_hook sysinit_premount + mount -a -t $NETFS -O no_netdev + run_hook sysinit_postmount stat_done }

@@ -502,13 +502,13 @@ fi set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" - #CONSOLEMAP in UTF-8 shouldn't be used - [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" - local i - for i in /dev/tty[0-9]*; do - /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ - $CONSOLEFONT -C ${i} &>/dev/null - done + #CONSOLEMAP in UTF-8 shouldn't be used + [[ $CONSOLEMAP && ${LOCALE,,} =~ utf ]] && CONSOLEMAP="" + local i + for i in /dev/tty[0-9]*; do + /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ + $CONSOLEFONT -C ${i} &>/dev/null + done if (( $? )); then stat_fail elif [[ $CONSOLEMAP ]]; then -- 1.7.1

I never really understood why we indented like this. Since we seem to be split 50/50, I'd argue that it might be better to just separate the code with whitespace, e.g.

# more code above here...

stat_busy foo bar baz stat_done

# more code below here...

This way, we don't have to worry about what happens when there could be a stat_fail somewhere in between that would make us have to wonder wtf to do with indenting.

d

The reason for the indentation was to make it obvious how stat_{busy,fail,done} match up. There were some errors due to mismatches before. I think it is relatively clear how to DTRT: if stat_fail/stat_done is one indentation level below stat_busy, then unindent it. Otherwise (e.g., if it is inside a conditional), don't. That said, I'm wondering if it wouldn't be clearer if we just used status, and when needed put these code blocks inside functions. -t

Kurt J. Bosch

26 Jun 26 Jun

1:10 a.m.

New subject: [arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

Tom Gundersen, 2011-06-25 21:10:

...

On Jun 25, 2011, at 20:11, Dave Reisner wrote:

...
On Sat, Jun 25, 2011 at 12:15:56PM +0200, Kurt J. Bosch wrote:

...
--- functions | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/functions b/functions index 61cf3dd..9c881c1 100644 --- a/functions +++ b/functions @@ -285,17 +285,17 @@ kill_everything() { # Terminate all processes stat_busy "Sending SIGTERM To Processes" run_hook "$1_prekillall" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args&>/dev/null - /bin/sleep 5 + local pid k5args="" + for pid in ${omit_pids[@]}; do + k5args+=" -o $pid" + done

I'd rather just see this whole chunk of code go away. Building the extra variable is moot when you can just use a PE:

killall 5 -15 ${omit_pids[@]/#/-o }

d

...
+ /sbin/killall5 -15 $k5args&>/dev/null + /bin/sleep 5 stat_done

stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args&>/dev/null - /bin/sleep 1 + /sbin/killall5 -9 $k5args&>/dev/null + /bin/sleep 1 stat_done

run_hook "$1_postkillall" @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { stat_busy "Checking Filesystems" - FSCK_OUT=/dev/stdout - FSCK_ERR=/dev/stdout - FSCK_FD= - FORCEFSCK= - [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline)&& FORCEFSCK="-- -f" - run_hook sysinit_prefsck - fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK>$FSCK_OUT 2>$FSCK_ERR + FSCK_OUT=/dev/stdout + FSCK_ERR=/dev/stdout + FSCK_FD= + FORCEFSCK= + [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline)&& FORCEFSCK="-- -f" + run_hook sysinit_prefsck + fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK>$FSCK_OUT 2>$FSCK_ERR local fsckret=$? if (( fsckret> 1 )); then stat_fail @@ -426,9 +426,9 @@ fsck_reboot() {

mount_all() { stat_busy "Mounting Local Filesystems" - run_hook sysinit_premount - mount -a -t $NETFS -O no_netdev - run_hook sysinit_postmount + run_hook sysinit_premount + mount -a -t $NETFS -O no_netdev + run_hook sysinit_postmount stat_done }

@@ -502,13 +502,13 @@ fi set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" - #CONSOLEMAP in UTF-8 shouldn't be used - [[ $CONSOLEMAP&& ${LOCALE,,} =~ utf ]]&& CONSOLEMAP="" - local i - for i in /dev/tty[0-9]*; do - /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ - $CONSOLEFONT -C ${i}&>/dev/null - done + #CONSOLEMAP in UTF-8 shouldn't be used + [[ $CONSOLEMAP&& ${LOCALE,,} =~ utf ]]&& CONSOLEMAP="" + local i + for i in /dev/tty[0-9]*; do + /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ + $CONSOLEFONT -C ${i}&>/dev/null + done if (( $? )); then stat_fail elif [[ $CONSOLEMAP ]]; then -- 1.7.1

I never really understood why we indented like this. Since we seem to be split 50/50, I'd argue that it might be better to just separate the code with whitespace, e.g.

# more code above here...

stat_busy foo bar baz stat_done

# more code below here...

This way, we don't have to worry about what happens when there could be a stat_fail somewhere in between that would make us have to wonder wtf to do with indenting.

d

The reason for the indentation was to make it obvious how stat_{busy,fail,done} match up. There were some errors due to mismatches before.

I think it is relatively clear how to DTRT: if stat_fail/stat_done is one indentation level below stat_busy, then unindent it. Otherwise (e.g., if it is inside a conditional), don't.

That said, I'm wondering if it wouldn't be clearer if we just used status, and when needed put these code blocks inside functions.

-t

Hmm... Currently we have the checks for skipping the entire thing as well as the stat_busy messages within the functions. This makes sense for three reasons: a) It allows to disable say set_consolefont in a sysinit hook by just unsetting $CONSOLEFONT and then call it later via some rc.multi hook without the need to duplicate the checking code (which isn't always that simple). This actually happens to avoid destroying a splash screen right in the middle of boot up. b) It allows to override the check and also the message together with the code instead of the code block only. c) It allows to have more than one code block in one single function (like in kill_everything) So, when using status, we would end up with somthing like set_consolefont() { [[ $CONSOLEFONT ]] || return 0 status "Loading Console Font: $CONSOLEFONT" \ set_consolefont_inner } set_consolefont_inner() { ... } -- Kurt

Kurt J. Bosch

12:01 a.m.

New subject: [arch-projects] [initscripts] [PATCH 04/12] functions: fix indentation

Dave Reisner, 2011-06-25 20:11:

...

On Sat, Jun 25, 2011 at 12:15:56PM +0200, Kurt J. Bosch wrote:

...
--- functions | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/functions b/functions index 61cf3dd..9c881c1 100644 --- a/functions +++ b/functions @@ -285,17 +285,17 @@ kill_everything() { # Terminate all processes stat_busy "Sending SIGTERM To Processes" run_hook "$1_prekillall" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args&>/dev/null - /bin/sleep 5 + local pid k5args="" + for pid in ${omit_pids[@]}; do + k5args+=" -o $pid" + done

I'd rather just see this whole chunk of code go away. Building the extra variable is moot when you can just use a PE:

killall 5 -15 ${omit_pids[@]/#/-o }

d

Very good catch! Patch already on the way...

...

...
+ /sbin/killall5 -15 $k5args&>/dev/null + /bin/sleep 5 stat_done

stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args&>/dev/null - /bin/sleep 1 + /sbin/killall5 -9 $k5args&>/dev/null + /bin/sleep 1 stat_done

run_hook "$1_postkillall" @@ -371,13 +371,13 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { stat_busy "Checking Filesystems" - FSCK_OUT=/dev/stdout - FSCK_ERR=/dev/stdout - FSCK_FD= - FORCEFSCK= - [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline)&& FORCEFSCK="-- -f" - run_hook sysinit_prefsck - fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK>$FSCK_OUT 2>$FSCK_ERR + FSCK_OUT=/dev/stdout + FSCK_ERR=/dev/stdout + FSCK_FD= + FORCEFSCK= + [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline)&& FORCEFSCK="-- -f" + run_hook sysinit_prefsck + fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK>$FSCK_OUT 2>$FSCK_ERR local fsckret=$? if (( fsckret> 1 )); then stat_fail @@ -426,9 +426,9 @@ fsck_reboot() {

mount_all() { stat_busy "Mounting Local Filesystems" - run_hook sysinit_premount - mount -a -t $NETFS -O no_netdev - run_hook sysinit_postmount + run_hook sysinit_premount + mount -a -t $NETFS -O no_netdev + run_hook sysinit_postmount stat_done }

@@ -502,13 +502,13 @@ fi set_consolefont() { [[ $CONSOLEFONT ]] || return 0 stat_busy "Loading Console Font: $CONSOLEFONT" - #CONSOLEMAP in UTF-8 shouldn't be used - [[ $CONSOLEMAP&& ${LOCALE,,} =~ utf ]]&& CONSOLEMAP="" - local i - for i in /dev/tty[0-9]*; do - /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ - $CONSOLEFONT -C ${i}&>/dev/null - done + #CONSOLEMAP in UTF-8 shouldn't be used + [[ $CONSOLEMAP&& ${LOCALE,,} =~ utf ]]&& CONSOLEMAP="" + local i + for i in /dev/tty[0-9]*; do + /usr/bin/setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \ + $CONSOLEFONT -C ${i}&>/dev/null + done if (( $? )); then stat_fail elif [[ $CONSOLEMAP ]]; then -- 1.7.1

I never really understood why we indented like this. Since we seem to be split 50/50, I'd argue that it might be better to just separate the code with whitespace, e.g.

# more code above here...

stat_busy foo bar baz stat_done

# more code below here...

This way, we don't have to worry about what happens when there could be a stat_fail somewhere in between that would make us have to wonder wtf to do with indenting.

d

Indentations on stat_busy doesn't look to bad to me - at least they match those on status with line continuation. I guess it's just a matter of taste. Maybe we should vote? ;-) -- Kurt

Kurt J. Bosch

12:02 a.m.

New subject: [arch-projects] [initscripts] [PATCH] kill_everything: Simplify omit_pid handling

All credits go to: Dave Reisner --- functions | 8 ++------ 1 files changed, 2 insertions(+), 6 deletions(-) diff --git a/functions b/functions index 9f59f56..253a4d1 100644 --- a/functions +++ b/functions @@ -286,16 +286,12 @@ kill_everything() { run_hook "${1}_prekillall" stat_busy "Sending SIGTERM To Processes" - local pid k5args="" - for pid in ${omit_pids[@]}; do - k5args+=" -o $pid" - done - /sbin/killall5 -15 $k5args &>/dev/null + /sbin/killall5 -15 ${omit_pids[@]/#/-o } &>/dev/null /bin/sleep 5 stat_done stat_busy "Sending SIGKILL To Processes" - /sbin/killall5 -9 $k5args &>/dev/null + /sbin/killall5 -9 ${omit_pids[@]/#/-o } &>/dev/null /bin/sleep 1 stat_done -- 1.7.1

Kurt J. Bosch

25 Jun 25 Jun

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 05/12] fsck_all, kill_everything: Clean up run_hook positions

* run postfsck hook within stat block as for prefsck * run prekillall hook out of stat blocks as for postkillall (revert obsolete e39ec61b7d642b36368d84f240b96eeda3c43b2f) --- functions | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/functions b/functions index 9c881c1..eab2074 100644 --- a/functions +++ b/functions @@ -283,8 +283,9 @@ kill_everything() { done # Terminate all processes + run_hook "${1}_prekillall" + stat_busy "Sending SIGTERM To Processes" - run_hook "$1_prekillall" local pid k5args="" for pid in ${omit_pids[@]}; do k5args+=" -o $pid" @@ -298,7 +299,7 @@ kill_everything() { /bin/sleep 1 stat_done - run_hook "$1_postkillall" + run_hook "${1}_postkillall" } # Start/trigger UDev, load MODULES and settle UDev @@ -378,13 +379,13 @@ fsck_all() { [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" run_hook sysinit_prefsck fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR - local fsckret=$? + local fsckret=$? + run_hook sysinit_postfsck if (( fsckret > 1 )); then stat_fail else stat_done fi - run_hook sysinit_postfsck return $fsckret } -- 1.7.1

Dave Reisner

8:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 05/12] fsck_all, kill_everything: Clean up run_hook positions

On Sat, Jun 25, 2011 at 12:15:57PM +0200, Kurt J. Bosch wrote:

...

* run postfsck hook within stat block as for prefsck * run prekillall hook out of stat blocks as for postkillall (revert obsolete e39ec61b7d642b36368d84f240b96eeda3c43b2f) --- functions | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/functions b/functions index 9c881c1..eab2074 100644 --- a/functions +++ b/functions @@ -283,8 +283,9 @@ kill_everything() { done

# Terminate all processes + run_hook "${1}_prekillall" + stat_busy "Sending SIGTERM To Processes" - run_hook "$1_prekillall" local pid k5args="" for pid in ${omit_pids[@]}; do k5args+=" -o $pid" @@ -298,7 +299,7 @@ kill_everything() { /bin/sleep 1 stat_done

- run_hook "$1_postkillall" + run_hook "${1}_postkillall" }

variables can't start with a number, so the extra bracing isn't necessary.

...

# Start/trigger UDev, load MODULES and settle UDev @@ -378,13 +379,13 @@ fsck_all() { [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" run_hook sysinit_prefsck fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR - local fsckret=$? + local fsckret=$? + run_hook sysinit_postfsck if (( fsckret > 1 )); then stat_fail else stat_done fi - run_hook sysinit_postfsck return $fsckret }

-- 1.7.1

Not a fan of this. I'd rather let the pass/fail come through before user hooks are run. There's potential for insanity, should a user decide to muck with the $fsckret value. Please try not to mix whitespace with code changes, as well. d

Tom Gundersen

9:11 p.m.

New subject: [arch-projects] [initscripts] [PATCH 05/12] fsck_all, kill_everything: Clean up run_hook positions

Sent from my iPhone On Jun 25, 2011, at 20:15, Dave Reisner wrote:

...

On Sat, Jun 25, 2011 at 12:15:57PM +0200, Kurt J. Bosch wrote:

...
* run postfsck hook within stat block as for prefsck * run prekillall hook out of stat blocks as for postkillall (revert obsolete e39ec61b7d642b36368d84f240b96eeda3c43b2f) --- functions | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/functions b/functions index 9c881c1..eab2074 100644 --- a/functions +++ b/functions @@ -283,8 +283,9 @@ kill_everything() { done

# Terminate all processes + run_hook "${1}_prekillall" + stat_busy "Sending SIGTERM To Processes" - run_hook "$1_prekillall" local pid k5args="" for pid in ${omit_pids[@]}; do k5args+=" -o $pid" @@ -298,7 +299,7 @@ kill_everything() { /bin/sleep 1 stat_done

- run_hook "$1_postkillall" + run_hook "${1}_postkillall" }

variables can't start with a number, so the extra bracing isn't necessary.

...
# Start/trigger UDev, load MODULES and settle UDev @@ -378,13 +379,13 @@ fsck_all() { [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" run_hook sysinit_prefsck fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR - local fsckret=$? + local fsckret=$? + run_hook sysinit_postfsck if (( fsckret > 1 )); then stat_fail else stat_done fi - run_hook sysinit_postfsck return $fsckret }

-- 1.7.1

Not a fan of this. I'd rather let the pass/fail come through before user hooks are run. There's potential for insanity, should a user decide to muck with the $fsckret value.

Please try not to mix whitespace with code changes, as well.

d

Kurt J. Bosch

26 Jun 26 Jun

12:32 a.m.

New subject: [arch-projects] [initscripts] [PATCH 05/12] fsck_all, kill_everything: Clean up run_hook positions

Dave Reisner, 2011-06-25 20:15:

...

On Sat, Jun 25, 2011 at 12:15:57PM +0200, Kurt J. Bosch wrote:

...
* run postfsck hook within stat block as for prefsck * run prekillall hook out of stat blocks as for postkillall (revert obsolete e39ec61b7d642b36368d84f240b96eeda3c43b2f) --- functions | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/functions b/functions index 9c881c1..eab2074 100644 --- a/functions +++ b/functions @@ -283,8 +283,9 @@ kill_everything() { done

# Terminate all processes + run_hook "${1}_prekillall" + stat_busy "Sending SIGTERM To Processes" - run_hook "$1_prekillall" local pid k5args="" for pid in ${omit_pids[@]}; do k5args+=" -o $pid" @@ -298,7 +299,7 @@ kill_everything() { /bin/sleep 1 stat_done

- run_hook "$1_postkillall" + run_hook "${1}_postkillall" }

variables can't start with a number, so the extra bracing isn't necessary.

Yes i know, but there are more things (like quoting within double brackets) which aren't needed, but might look better at least for the less experienced people.

...

...
# Start/trigger UDev, load MODULES and settle UDev @@ -378,13 +379,13 @@ fsck_all() { [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline)&& FORCEFSCK="-- -f" run_hook sysinit_prefsck fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK>$FSCK_OUT 2>$FSCK_ERR - local fsckret=$? + local fsckret=$? + run_hook sysinit_postfsck if (( fsckret> 1 )); then stat_fail else stat_done fi - run_hook sysinit_postfsck return $fsckret }

-- 1.7.1

Not a fan of this. I'd rather let the pass/fail come through before user hooks are run. There's potential for insanity, should a user decide to muck with the $fsckret value.

$fsckret has to be exposed to the hook to allow a splash system to switch back to the text console for showing the reboot messages when fsck failed. I'll just define it read-only.

...

Please try not to mix whitespace with code changes, as well.

d

OK. I promises to do my very best. -- Kurt

Kurt J. Bosch

12:33 a.m.

New subject: [arch-projects] [initscripts] [PATCH] fsck_all: Define $fsckret local variable read-only because exposed to hook

Credits to: Dave Reisner --- functions | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/functions b/functions index 253a4d1..ab7928f 100644 --- a/functions +++ b/functions @@ -372,7 +372,7 @@ fsck_all() { [[ -f /forcefsck ]] || in_array forcefsck $(< /proc/cmdline) && FORCEFSCK="-- -f" run_hook sysinit_prefsck fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR - local fsckret=$? + local -r fsckret=$? run_hook sysinit_postfsck # Ignore condition 'FS errors corrected' (( (fsckret | 1) == 1 )) && stat_done || stat_fail -- 1.7.1

Kurt J. Bosch

25 Jun 25 Jun

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 06/12] ck_depends, ck_pidfile, do_unlock, kill_everything: add missing local statements

--- functions | 6 +++++- rc.sysinit | 12 ++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/functions b/functions index eab2074..5f5d97e 100644 --- a/functions +++ b/functions @@ -210,7 +210,8 @@ start_daemon() { # Never use this function, it causes daemons to be stoped in the wrong order. # The only way to start a daemon at boot is to add it to the DAEMONS array. ck_depends() { - for daemon in "$@"; do + local daemon + for daemon; do ck_daemon "$daemon" && start_daemon "$daemon" done } @@ -251,6 +252,7 @@ get_pid() { # Check if PID-file $1 is still the active PID-file for command $2 ck_pidfile() { if [[ -f "$1" ]]; then + local fpid ppid read -r fpid <"$1" ppid=$(get_pid $2) [[ "$fpid" = "$ppid" ]] && return 0 @@ -270,6 +272,7 @@ kill_everything() { # $1 = where we are being called from. # This is used to determine which hooks to run. # Find daemons NOT in the DAEMONS array. Shut these down first + local deamon for daemon in /run/daemons/*; do [[ -f $daemon ]] || continue daemon=${daemon##*/} @@ -277,6 +280,7 @@ kill_everything() { done # Shutdown daemons in reverse order + local i for (( i=${#DAEMONS[@]}-1; i>=0; i-- )); do [[ ${DAEMONS[i]} = '!'* ]] && continue ck_daemon ${DAEMONS[i]#@} || stop_daemon ${DAEMONS[i]#@} diff --git a/rc.sysinit b/rc.sysinit index 5edffe1..5910e3b 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -138,12 +138,12 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then printf "\nOpening '$1' volume:\n" $CS $4 $open "$a" "$b" < /dev/console;; /dev*) - ckdev=${3%%:*} - cka=${3#*:} - ckb=${cka#*:} - cka=${cka%:*} - ckfile=/dev/ckfile - ckdir=/dev/ckdir + local ckdev=${3%%:*} + local cka=${3#*:} + local ckb=${cka#*:} + local cka=${cka%:*} + local ckfile=/dev/ckfile + local ckdir=/dev/ckdir case ${cka} in *[!0-9]*) # Use a file on the device -- 1.7.1

Kurt J. Bosch

12:15 p.m.

New subject: [arch-projects] [initscripts] [PATCH 07/12] Move check on executable fsck into fsck_all() for consistency

--- functions | 1 + rc.sysinit | 10 ++++------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/functions b/functions index 5f5d97e..6b7c8fc 100644 --- a/functions +++ b/functions @@ -375,6 +375,7 @@ NETFS="nonfs,nonfs4,nosmbfs,nocifs,nocodafs,noncpfs,nosysfs,noshfs,nofuse,nofuse # Check local filesystems fsck_all() { + [[ -x $(type -P fsck) ]] || return 0 stat_busy "Checking Filesystems" FSCK_OUT=/dev/stdout FSCK_ERR=/dev/stdout diff --git a/rc.sysinit b/rc.sysinit index 5910e3b..9f44848 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -186,12 +186,10 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then fi fi -if [[ -x $(type -P fsck) ]]; then - # Check filesystems - fsck_all - # Single-user login and/or automatic reboot if needed - fsck_reboot $? -fi +# Check filesystems +fsck_all +# Single-user login and/or automatic reboot if needed +fsck_reboot $? status "Remounting Root Read/Write" \ mount -n -o remount,rw / -- 1.7.1

Kurt J. Bosch

12:16 p.m.

New subject: [arch-projects] [initscripts] [PATCH 08/12] activate_vgs, fsck_reboot: fix initial return statements for consistecy

* return 0 if skipped as always * get rid of if-construct using '&&' --- functions | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/functions b/functions index 6b7c8fc..564ea4c 100644 --- a/functions +++ b/functions @@ -334,7 +334,7 @@ udevd_modprobe() { } activate_vgs() { - [[ $USELVM = [yY][eE][sS] && -x $(type -P lvm) && -d /sys/block ]] || return + [[ $USELVM = [yY][eE][sS] && -x $(type -P lvm) && -d /sys/block ]] || return 0 stat_busy "Activating LVM2 groups" # Kernel 2.6.x, LVM2 groups /sbin/modprobe -q dm-mod 2>/dev/null @@ -398,9 +398,8 @@ fsck_all() { fsck_reboot() { # $1 = exit code returned by fsck # Ignore conditions 'FS errors corrected' and 'Cancelled by the user' - if (( ($1 | 33) == 33 )); then - return 0 - elif (( $1 & 2 )); then + (( ($1 | 33) == 33 )) && return 0 + if (( $1 & 2 )); then echo echo "********************** REBOOT REQUIRED *********************" echo "* *" -- 1.7.1

Kurt J. Bosch

12:16 p.m.

New subject: [arch-projects] [initscripts] [PATCH 09/12] udevd_modprobe: Get rid of simple if-constuct by using '&&'

--- functions | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/functions b/functions index 564ea4c..1c6a780 100644 --- a/functions +++ b/functions @@ -320,9 +320,8 @@ udevd_modprobe() { stat_done # Load modules from the MODULES array defined in rc.conf - if [[ -f /proc/modules ]] && (( ${#MODULES[*]} )); then + [[ -f /proc/modules ]] && (( ${#MODULES[*]} )) && status "Loading Modules" modprobe -ab "${MODULES[@]}" - fi status "Waiting for UDev uevents to be processed" \ udevadm settle --timeout=${UDEV_TIMEOUT:-30} -- 1.7.1

Kurt J. Bosch

12:16 p.m.

New subject: [arch-projects] [initscripts] [PATCH 10/12] Get rid of more simple if-constucts by using '&&' and '||'

--- functions | 24 +++++++----------------- rc.sysinit | 22 ++++++---------------- 2 files changed, 13 insertions(+), 33 deletions(-) diff --git a/functions b/functions index 1c6a780..86b9602 100644 --- a/functions +++ b/functions @@ -237,11 +237,7 @@ status_stopped() { } ck_status() { - if ! ck_daemon "$1"; then - status_started - else - status_stopped - fi + ! ck_daemon "$1" && status_started || status_stopped } # Return PID of $1 @@ -335,13 +331,10 @@ udevd_modprobe() { activate_vgs() { [[ $USELVM = [yY][eE][sS] && -x $(type -P lvm) && -d /sys/block ]] || return 0 stat_busy "Activating LVM2 groups" - # Kernel 2.6.x, LVM2 groups - /sbin/modprobe -q dm-mod 2>/dev/null - if /sbin/vgchange --sysinit -a y >/dev/null; then - stat_done - else - stat_fail - fi + # Kernel 2.6.x, LVM2 groups + /sbin/modprobe -q dm-mod 2>/dev/null + /sbin/vgchange --sysinit -a y >/dev/null + (( $? == 0 )) && stat_done || stat_fail } # Arch cryptsetup packages traditionally contained the binaries @@ -385,11 +378,8 @@ fsck_all() { fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR local fsckret=$? run_hook sysinit_postfsck - if (( fsckret > 1 )); then - stat_fail - else - stat_done - fi + # Ignore condition 'FS errors corrected' + (( (fsckret | 1) == 1 )) && stat_done || stat_fail return $fsckret } diff --git a/rc.sysinit b/rc.sysinit index 9f44848..097b38a 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -68,11 +68,7 @@ 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). - if TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS; then - stat_done - else - stat_fail - fi + TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS && stat_done || stat_fail fi # Start/trigger UDev, load MODULES and settle UDev @@ -175,15 +171,9 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then return $failed } crypto_unlocked=0 - if read_crypttab do_unlock; then - stat_done - else - stat_fail - fi - if (( crypto_unlocked == 1 )); then - # Maybe someone has LVM on an encrypted block device - activate_vgs - fi + read_crypttab do_unlock && stat_done || stat_fail + # Maybe someone has LVM on an encrypted block device + (( crypto_unlocked == 1 )) && activate_vgs fi # Check filesystems @@ -202,7 +192,7 @@ if [[ ! -L /etc/mtab ]]; then else cat /proc/mounts >| /etc/mtab fi - if (( $? == 0 )); then stat_done; else stat_fail; fi + (( $? == 0 )) && stat_done || stat_fail fi # now mount all the local filesystems @@ -278,7 +268,7 @@ stat_busy "Saving dmesg Log" else install -Tm 0644 <( dmesg ) /var/log/dmesg.log fi -if (( $? == 0 )); then stat_done; else stat_fail; fi +(( $? == 0 )) && stat_done || stat_fail run_hook sysinit_end -- 1.7.1

Dave Reisner

8:34 p.m.

New subject: [arch-projects] [initscripts] [PATCH 10/12] Get rid of more simple if-constucts by using '&&' and '||'

On Sat, Jun 25, 2011 at 12:16:02PM +0200, Kurt J. Bosch wrote:

...

--- functions | 24 +++++++----------------- rc.sysinit | 22 ++++++---------------- 2 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/functions b/functions index 1c6a780..86b9602 100644 --- a/functions +++ b/functions @@ -237,11 +237,7 @@ status_stopped() { }

ck_status() { - if ! ck_daemon "$1"; then - status_started - else - status_stopped - fi + ! ck_daemon "$1" && status_started || status_stopped }

# Return PID of $1 @@ -335,13 +331,10 @@ udevd_modprobe() { activate_vgs() { [[ $USELVM = [yY][eE][sS] && -x $(type -P lvm) && -d /sys/block ]] || return 0 stat_busy "Activating LVM2 groups" - # Kernel 2.6.x, LVM2 groups - /sbin/modprobe -q dm-mod 2>/dev/null - if /sbin/vgchange --sysinit -a y >/dev/null; then - stat_done - else - stat_fail - fi + # Kernel 2.6.x, LVM2 groups + /sbin/modprobe -q dm-mod 2>/dev/null + /sbin/vgchange --sysinit -a y >/dev/null + (( $? == 0 )) && stat_done || stat_fail }

# Arch cryptsetup packages traditionally contained the binaries @@ -385,11 +378,8 @@ fsck_all() { fsck -A -T -C$FSCK_FD -a -t "$NETFS,noopts=_netdev" $FORCEFSCK >$FSCK_OUT 2>$FSCK_ERR local fsckret=$? run_hook sysinit_postfsck - if (( fsckret > 1 )); then - stat_fail - else - stat_done - fi + # Ignore condition 'FS errors corrected' + (( (fsckret | 1) == 1 )) && stat_done || stat_fail return $fsckret }

diff --git a/rc.sysinit b/rc.sysinit index 9f44848..097b38a 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -68,11 +68,7 @@ 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). - if TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS; then - stat_done - else - stat_fail - fi + TZ=$TIMEZONE hwclock $HWCLOCK_PARAMS && stat_done || stat_fail fi

Whitespace error here?

...

# Start/trigger UDev, load MODULES and settle UDev @@ -175,15 +171,9 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then return $failed } crypto_unlocked=0 - if read_crypttab do_unlock; then - stat_done - else - stat_fail - fi - if (( crypto_unlocked == 1 )); then - # Maybe someone has LVM on an encrypted block device - activate_vgs - fi + read_crypttab do_unlock && stat_done || stat_fail + # Maybe someone has LVM on an encrypted block device + (( crypto_unlocked == 1 )) && activate_vgs fi

# Check filesystems @@ -202,7 +192,7 @@ if [[ ! -L /etc/mtab ]]; then else cat /proc/mounts >| /etc/mtab fi - if (( $? == 0 )); then stat_done; else stat_fail; fi + (( $? == 0 )) && stat_done || stat_fail fi

# now mount all the local filesystems @@ -278,7 +268,7 @@ stat_busy "Saving dmesg Log" else install -Tm 0644 <( dmesg ) /var/log/dmesg.log fi -if (( $? == 0 )); then stat_done; else stat_fail; fi +(( $? == 0 )) && stat_done || stat_fail

run_hook sysinit_end

-- 1.7.1

Kurt J. Bosch

26 Jun 26 Jun

1:21 a.m.

New subject: [arch-projects] [initscripts] [PATCH] rc.sysinit: fix whitespace errors

--- rc.sysinit | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rc.sysinit b/rc.sysinit index 75f8095..a6a5fa5 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -62,10 +62,10 @@ if [[ $HWCLOCK_PARAMS ]]; then # 2. Filesystem checks can depend on system time # 3. This also sets the kernel time zone, used by e.g. vfat # If TIMEZONE is not set in rc.conf, the timezone stored in /etc/localtime - # 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 + # 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 || stat_fail fi # Start/trigger UDev, load MODULES and settle UDev @@ -168,7 +168,7 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#] /etc/crypttab; then return $failed } crypto_unlocked=0 - read_crypttab do_unlock && stat_done || stat_fail + read_crypttab do_unlock && stat_done || stat_fail # Maybe someone has LVM on an encrypted block device (( crypto_unlocked == 1 )) && activate_vgs fi -- 1.7.1

Kurt J. Bosch

25 Jun 25 Jun

12:16 p.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ # sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS] && $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

Dave Reisner

8 p.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

On Sat, Jun 25, 2011 at 12:16:03PM +0200, Kurt J. Bosch wrote:

...

--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ

# sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS] && $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

I'm all for simplifying things, but this will unset _anything_ starting with LC_ which could conceivably be something a calling script cares about. dave

Kurt J. Bosch

11:40 p.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

Dave Reisner, 2011-06-25 20:00:

...

On Sat, Jun 25, 2011 at 12:16:03PM +0200, Kurt J. Bosch wrote:

...
--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ

# sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS]&& $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

I'm all for simplifying things, but this will unset _anything_ starting with LC_ which could conceivably be something a calling script cares about.

dave

NACK We talk about rc scripts and daemons here don't we? At the point where functions are sourced they should not care about anything else than what is normally exported by init or supported in rc.conf. Ideally we should unset _everything_ other to avoid unexpected daemon restart behavior. Things special to a daemon script should go into /etc/conf.d/$daemon which is sourced after functions. -- Kurt

Tom Gundersen

11:57 p.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

On Sat, Jun 25, 2011 at 11:40 PM, Kurt J. Bosch wrote:

...

Dave Reisner, 2011-06-25 20:00:

...
On Sat, Jun 25, 2011 at 12:16:03PM +0200, Kurt J. Bosch wrote:

...
--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ

# sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS]&& $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

I'm all for simplifying things, but this will unset _anything_ starting with LC_ which could conceivably be something a calling script cares about.

dave

NACK We talk about rc scripts and daemons here don't we? At the point where functions are sourced they should not care about anything else than what is normally exported by init or supported in rc.conf. Ideally we should unset _everything_ other to avoid unexpected daemon restart behavior. Things special to a daemon script should go into /etc/conf.d/$daemon which is sourced after functions.

I think it would be best if we were able to unset all variables except for the ones we explicitly want. I have been meaning to look into using /etc/initscript to set/unset system-wide variables, but I haven't had the time to check if this really works the way I want. If I understand correctly, the variables exported in /etc/initscript will be inherited by all processes on the system, (unless a child unsets them of course). This would allow us to put the LC_* and PATH in there (rather than in /etc/profile and /etc/profile.d/locale.sh), and things should JustWork(TM). At the moment it looks like agetty is clearing some (if not all) the env var's it is passed, but I have not yet found out exactly how it works. -t

Kurt J. Bosch

26 Jun 26 Jun

1:40 a.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

Tom Gundersen, 2011-06-25 23:57:

...

On Sat, Jun 25, 2011 at 11:40 PM, Kurt J. Bosch wrote:

...
Dave Reisner, 2011-06-25 20:00:

...
On Sat, Jun 25, 2011 at 12:16:03PM +0200, Kurt J. Bosch wrote:

...
--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ

# sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS]&& $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

I'm all for simplifying things, but this will unset _anything_ starting with LC_ which could conceivably be something a calling script cares about.

dave

NACK We talk about rc scripts and daemons here don't we? At the point where functions are sourced they should not care about anything else than what is normally exported by init or supported in rc.conf. Ideally we should unset _everything_ other to avoid unexpected daemon restart behavior. Things special to a daemon script should go into /etc/conf.d/$daemon which is sourced after functions.

I think it would be best if we were able to unset all variables except for the ones we explicitly want.

I have been meaning to look into using /etc/initscript to set/unset system-wide variables, but I haven't had the time to check if this really works the way I want.

If I understand correctly, the variables exported in /etc/initscript will be inherited by all processes on the system, (unless a child unsets them of course). This would allow us to put the LC_* and PATH in there (rather than in /etc/profile and /etc/profile.d/locale.sh), and things should JustWork(TM).

AFAIKS you can only unset things coming from init (or initrd) in /etc/initscript. That wouldn't solve possible problems caused by a different environment when restarting a daemon manually or from a pm-utils hook.

...

At the moment it looks like agetty is clearing some (if not all) the env var's it is passed, but I have not yet found out exactly how it works.

I think it should just keep the locale settings intact for the login prompt and the rest should be set up afterwards, not?

...

-t

-- Kurt

Seblu

7:43 p.m.

New subject: [arch-projects] [initscripts] [PATCH 11/12] Simplify LC_* unsetting

On Sat, Jun 25, 2011 at 11:57 PM, Tom Gundersen wrote:

...

On Sat, Jun 25, 2011 at 11:40 PM, Kurt J. Bosch wrote:

...
Dave Reisner, 2011-06-25 20:00:

...
On Sat, Jun 25, 2011 at 12:16:03PM +0200, Kurt J. Bosch wrote:

...
--- functions | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/functions b/functions index 86b9602..2e4ba42 100644 --- a/functions +++ b/functions @@ -62,9 +62,7 @@ unset TERM_COLORS unset TZ

# sanitize the locale settins -unset LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY \ - LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE \ - LC_MEASUREMENT LC_IDENTIFICATION LC_ALL +unset "${!LC_@}" if [[ $DAEMON_LOCALE = [yY][eE][sS]&& $LOCALE ]]; then export LANG="${LOCALE}" else -- 1.7.1

I'm all for simplifying things, but this will unset _anything_ starting with LC_ which could conceivably be something a calling script cares about.

dave

NACK i agree with dave. We should just remove what we want or doing what tom propose (cleaning all and add what we want).

...

...
We talk about rc scripts and daemons here don't we? At the point where functions are sourced they should not care about anything else than what is normally exported by init or supported in rc.conf. Ideally we should unset _everything_ other to avoid unexpected daemon restart behavior. Things special to a daemon script should go into /etc/conf.d/$daemon which is sourced after functions. As far i know, there is no obligation that conf.d/ scripts is bash script.

...

I think it would be best if we were able to unset all variables except for the ones we explicitly want. init already clean all vars (excep those in man) when runnint rc.* rc.d clean the env like init do it. It's one of the avantage of using rc.d. But this should be done in functions...

...

I have been meaning to look into using /etc/initscript to set/unset system-wide variables, but I haven't had the time to check if this really works the way I want.

If I understand correctly, the variables exported in /etc/initscript will be inherited by all processes on the system, (unless a child unsets them of course). This would allow us to put the LC_* and PATH in there (rather than in /etc/profile and /etc/profile.d/locale.sh), and things should JustWork(TM).

yeah, this probably help with our locales inheritance issue !

...

At the moment it looks like agetty is clearing some (if not all) the env var's it is passed, but I have not yet found out exactly how it works.

-t

-- Sébastien Luttringer www.seblu.net

Kurt J. Bosch

25 Jun 25 Jun

12:16 p.m.

New subject: [arch-projects] [initscripts] [PATCH 12/12] Move export PATH into functions

Sanitze the PATH for daemons also when not run from rc.shutdown. --- functions | 4 +++- rc.shutdown | 2 -- rc.sysinit | 3 --- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/functions b/functions index 2e4ba42..9f59f56 100644 --- a/functions +++ b/functions @@ -2,7 +2,8 @@ # initscripts functions # -# width: +# sanitize PATH (will be overridden later when /etc/profile is sourced, but is useful for UDev) +export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" if [[ $1 == "start" ]]; then if [[ $STARTING ]]; then @@ -12,6 +13,7 @@ if [[ $1 == "start" ]]; then fi fi +# width: calc_columns () { STAT_COL=80 if [[ ! -t 1 ]]; then diff --git a/rc.shutdown b/rc.shutdown index 9eb0b31..d16c66a 100755 --- a/rc.shutdown +++ b/rc.shutdown @@ -6,8 +6,6 @@ . /etc/rc.conf . /etc/rc.d/functions -export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - run_hook shutdown_start # avoid staircase effect diff --git a/rc.sysinit b/rc.sysinit index 097b38a..4073fb9 100755 --- a/rc.sysinit +++ b/rc.sysinit @@ -13,9 +13,6 @@ printsep run_hook sysinit_start -# export standard PATH (will be overridden later when /etc/profile is sourced, but is useful for UDev) -export PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - # 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 -- 1.7.1

4694

Age (days ago)

4695

Last active (days ago)

List overview

Download

30 comments

4 participants

participants (4)

Dave Reisner
Kurt J. Bosch
Seblu
Tom Gundersen