On Tue, Jul 12, 2011 at 10:58:36AM +0200, Kurt J. Bosch wrote:
> The rules should be:
>
> * Use quotes when literal strings are involved.
> * Use quotes when ever using a substitution which might produce blanks
> as a possitional parameter unless word splitting is intended.
> * Don't use quotes for substitutions where _clearly_ not needed.
> That is within '[[ ]]', assignments and 'case ... in' (where no word
> splitting is performed) and also when using local variables which
> _obviously_ don't contain blanks.
The RHS of a bash test is subject to glob matching and should almost
always be quoted.
$ pattern=foo*
$ [[ foobar = $pattern ]]; echo $?
0
$ [[ foobar = "$pattern" ]]; echo $?
1
So this isn't really black and white. Fortunately, this doesn't appear
to be an issue.
Other comments inlined.
> ---
> functions | 50 +++++++++++++++++++++++++-------------------------
> rc.sysinit | 22 +++++++++++-----------
> 2 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/functions b/functions
> index 722098d..bdb8529 100644
> --- a/functions
> +++ b/functions
> @@ -20,9 +20,9 @@ calc_columns () {
> USECOLOR=""
> elif [[ -t 0 ]]; then
> # stty will fail when stdin isn't a terminal
> - STAT_COL="$(stty size)"
> + STAT_COL=$(stty size)
> # stty gives "rows cols"; strip the rows number, we just want
> columns
> - STAT_COL="${STAT_COL##* }"
> + STAT_COL=${STAT_COL##* }
> elif tput cols &>/dev/null; then
> # is /usr/share/terminfo already mounted, and TERM recognized?
> STAT_COL=$(tput cols)
> @@ -50,7 +50,7 @@ calc_columns () {
> calc_columns
>
> # disable colors on broken terminals
> -TERM_COLORS="$(tput colors 2>/dev/null)"
> +TERM_COLORS=$(tput colors 2>/dev/null)
> if (( $? != 3 )); then
> case $TERM_COLORS in
> *[!0-9]*) USECOLOR="";;
> @@ -76,16 +76,16 @@ fi
> # set colors
> if [[ $USECOLOR = [yY][eE][sS] ]]; then
> if tput setaf 0 &>/dev/null; then
> - C_CLEAR="$(tput sgr0)" # clear text
> - C_MAIN="${C_CLEAR}$(tput bold)" # main text
> - C_OTHER="${C_MAIN}$(tput setaf 4)" # prefix & brackets
> - C_SEPARATOR="${C_MAIN}$(tput setaf 0)" # separator
> - C_BUSY="${C_CLEAR}$(tput setaf 6)" # busy
> - C_FAIL="${C_MAIN}$(tput setaf 1)" # failed
> - C_DONE="${C_MAIN}" # completed
> - C_BKGD="${C_MAIN}$(tput setaf 5)" # backgrounded
> - C_H1="${C_MAIN}" # highlight text 1
> - C_H2="${C_MAIN}$(tput setaf 6)" # highlight text 2
> + C_CLEAR=$(tput sgr0) # clear text
> + C_MAIN=${C_CLEAR}$(tput bold) # main text
> + C_OTHER=${C_MAIN}$(tput setaf 4) # prefix & brackets
> + C_SEPARATOR=${C_MAIN}$(tput setaf 0) # separator
> + C_BUSY=${C_CLEAR}$(tput setaf 6) # busy
> + C_FAIL=${C_MAIN}$(tput setaf 1) # failed
> + C_DONE=${C_MAIN} # completed
> + C_BKGD=${C_MAIN}$(tput setaf 5) # backgrounded
> + C_H1=${C_MAIN} # highlight text 1
> + C_H2=${C_MAIN}$(tput setaf 6) # highlight text 2
> else
> C_CLEAR="\e[m" # clear text
> C_MAIN="\e[;1m" # main text
> @@ -93,9 +93,9 @@ if [[ $USECOLOR = [yY][eE][sS] ]]; then
> C_SEPARATOR="\e[1;30m" # separator
> C_BUSY="\e[;36m" # busy
> C_FAIL="\e[1;31m" # failed
> - C_DONE="${C_MAIN}" # completed
> + C_DONE=${C_MAIN} # completed
> C_BKGD="\e[1;35m" # backgrounded
> - C_H1="${C_MAIN}" # highlight text 1
> + C_H1=${C_MAIN} # highlight text 1
> C_H2="\e[1;36m" # highlight text 2
> fi
> fi
> @@ -198,7 +198,7 @@ have_daemon() {
> ck_autostart() {
> local d
> for d in "${DAEMONS[@]}"; do
> - [[ "$1" = ${d#@} ]] && return 1
> + [[ $1 = ${d#@} ]] && return 1
> done
> return 0
> }
> @@ -247,11 +247,11 @@ get_pid() {
>
> # Check if PID-file $1 is still the active PID-file for command $2
> ck_pidfile() {
> - if [[ -f "$1" ]]; then
> + if [[ -f $1 ]]; then
> local fpid ppid
> read -r fpid <"$1"
> - ppid=$(get_pid $2)
> - [[ "$fpid" = "$ppid" ]] && return 0
> + ppid=$(get_pid "$2")
> + [[ $fpid = $ppid ]] && return 0
> fi
> return 1
> }
> @@ -279,7 +279,7 @@ stop_all_daemons() {
> local i
> for (( i=${#DAEMONS[@]}-1; i>=0; i-- )); do
> [[ ${DAEMONS[i]} = '!'* ]] && continue
> - ck_daemon ${DAEMONS[i]#@} || stop_daemon ${DAEMONS[i]#@}
> + ck_daemon "${DAEMONS[i]#@}" || stop_daemon "${DAEMONS[i]#@}"
> done
> }
>
> @@ -325,7 +325,7 @@ udevd_modprobe() {
> status "Loading Modules" modprobe -ab "${MODULES[@]}"
>
> status "Waiting for UDev uevents to be processed" \
> - udevadm settle --timeout=${UDEV_TIMEOUT:-30}
> + udevadm settle --timeout="${UDEV_TIMEOUT:-30}"
Why does this warrant quoting? passing --timeout= is harmless. If a user
sets anything but a number here it's a syntax error, and udevadm will,
furthermore, ignore the argument that it doesn't understand when
multiple words are assigned.
>
> run_hook "$1_udevsettled"
>
> @@ -374,7 +374,7 @@
> NETFS="nfs,nfs4,smbfs,cifs,codafs,ncpfs,shfs,fuse,fuseblk,glusterfs,davfs,fuse.g
>
> # Check local filesystems
> fsck_all() {
> - fsck -A -T -C$FSCK_FD -a -t "no${NETFS//,/,no},noopts=_netdev"
> $FORCEFSCK
> + fsck -A -T -C"$FSCK_FD" -a -t "no${NETFS//,/,no},noopts=_netdev"
> $FORCEFSCK
> return $?
> }
>
> @@ -486,7 +486,7 @@ if (( RC_FUNCTIONS_HOOK_FUNCS_DEFINED != 1 )); then
>
> add_hook() {
> [[ $1 && $2 ]] || return 1
> - hook_funcs["$1"]+=" $2"
> + hook_funcs[$1]+=" $2"
> }
>
> run_hook() {
> @@ -509,8 +509,8 @@ set_consolefont() {
> [[ ${LOCALE,,} =~ utf ]] && CONSOLEMAP=""
> local i
> for i in /dev/tty[0-9]*; do
> - setfont ${CONSOLEMAP:+-m ${CONSOLEMAP}} \
> - $CONSOLEFONT -C ${i} &>/dev/null
> + setfont ${CONSOLEMAP:+-m "${CONSOLEMAP}"} \
> + "$CONSOLEFONT" -C ${i} &>/dev/null
> done
> if (( $? )); then
> false
> diff --git a/rc.sysinit b/rc.sysinit
> index 1516fba..839a787 100755
> --- a/rc.sysinit
> +++ b/rc.sysinit
> @@ -96,14 +96,14 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#]
> /etc/crypttab; then
> # $3 = password
> # $4 = options
> stat_append "${1}.."
> - local open=create a="$1" b="$2" failed=0
> + local open=create a=$1 b=$2 failed=0
> # Ordering of options is different if you are using
> LUKS vs. not.
> # Use ugly swizzling to deal with it.
> # isLuks only gives an exit code but no output to
> stdout or stderr.
> if $CS isLuks "$2" 2>/dev/null; then
> open=luksOpen
> - a="$2"
> - b="$1"
> + a=$2
> + b=$1
> fi
> case $3 in
> SWAP)
> @@ -123,13 +123,13 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#]
> /etc/crypttab; then
> fi
> if (( _overwriteokay == 0 )); then
> false
> - elif $CS -d /dev/urandom $4 $open "$a"
> "$b" >/dev/null; then
> + elif $CS -d /dev/urandom "$4" $open
> "$a" "$b" >/dev/null; then
> stat_append "creating
> swapspace.."
> - mkswap -f -L $1 /dev/mapper/$1
> >/dev/null
> + mkswap -f -L $1
> /dev/mapper/"$1" >/dev/null
> fi;;
> ASK)
> printf "\nOpening '$1' volume:\n"
> - $CS $4 $open "$a" "$b" < /dev/console;;
> + $CS "$4" $open "$a" "$b" <
> /dev/console;;
> /dev*)
> local ckdev=${3%%:*}
> local cka=${3#*:}
> @@ -151,13 +151,13 @@ if [[ -f /etc/crypttab && $CS ]] && grep -q ^[^#]
> /etc/crypttab; then
> # cka is numeric:
> cka=offset, ckb=length
> dd if=${ckdev}
> of=${ckfile} bs=1 skip=${cka} count=${ckb} >/dev/null 2>&1;;
> esac
> - $CS -d ${ckfile} $4 $open "$a" "$b"
> >/dev/null
> + $CS -d ${ckfile} "$4" $open "$a" "$b"
> >/dev/null
> dd if=/dev/urandom of=${ckfile} bs=1
> count=$(stat -c %s ${ckfile}) conv=notrunc >/dev/null 2>&1
> rm ${ckfile};;
> /*)
> - $CS -d "$3" $4 $open "$a" "$b"
> >/dev/null;;
> + $CS -d "$3" "$4" $open "$a" "$b"
> >/dev/null;;
> *)
> - echo "$3" | $CS $4 $open "$a" "$b"
> >/dev/null;;
> + echo "$3" | $CS "$4" $open "$a" "$b"
> >/dev/null;;
> esac
> if (( $? )); then
> failed=1
> @@ -179,7 +179,7 @@ declare -r FORCEFSCK
> run_hook sysinit_prefsck
> if [[ -x $(type -P fsck) ]]; then
> stat_busy "Checking Filesystems"
> - fsck_all >|${FSCK_OUT:-/dev/stdout} 2>|${FSCK_ERR:-/dev/stdout}
> + fsck_all >|"${FSCK_OUT:-/dev/stdout}"
> 2>|"${FSCK_ERR:-/dev/stdout}"
> declare -r fsckret=$?
> (( fsckret <= 1 )) && stat_done || stat_fail
> else
> @@ -261,7 +261,7 @@ else
> stat_done
> fi
> [[ $KEYMAP ]] &&
> - status "Loading Keyboard Map: $KEYMAP" loadkeys -q $KEYMAP
> + status "Loading Keyboard Map: $KEYMAP" loadkeys -q "$KEYMAP"
KEYMAP might be multiple words, intentionally, to load multiple keymaps.
We cannot quote this.
https://bugs.archlinux.org/task/23373
>
> # Set console font if required
> set_consolefont
> --
> 1.7.1
>
