Tom Gundersen, 2011-06-25 21:10:
On Jun 25, 2011, at 20:11, Dave Reisner<d@falconindy.com> 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