[arch-projects] [initscripts] [PATCH 11/13] functions: Fix/unify quoting
Kurt J. Bosch
kjb-temp-2009 at alpenjodel.de
Tue Jul 12 12:22:20 EDT 2011
Dave Reisner, 2011-07-12 12:19:
> 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.
Good catch. Fixed this in the new thread together with those below to
avoid this will ever be forgotten.
>
> 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.
>
OK. You convinced me.
>>
>> 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
>
Oops, thank you for clarifying this.
>
>>
>> # Set console font if required
>> set_consolefont
>> --
>> 1.7.1
>>
--
Kurt
More information about the arch-projects
mailing list