[iproute PATCH v2 0/2] Covscan: Shell script fixes

[iproute PATCH v2 0/2] Covscan: Shell script fixes

[Phil Sutter]

This series collects patches from v1 which deal with programming
mistakes in shell scripts.

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (2):
  examples: Some shell fixes to cbq.init
  ifcfg: Quote left-hand side of [ ] expression

 examples/cbq.init-v0.7.3 | 24 ++++++++++++------------
 ip/ifcfg                 |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.13.1

[iproute PATCH v2 1/2] examples: Some shell fixes to cbq.init

[Phil Sutter]

This addresses the following issues:

- $@ is an array, so don't use it in quoted strings - use $* instead.

- Add missing quotes to components of [ ] expressions. These are not
  strictly necessary since the output of 'wc -l' should be a single word
  only, but in case of errors, bash prints "integer expression expected"
  instead of "too many arguments".

- Use -print0/-0 when piping from find to xargs to allow for filenames
  which contain whitespace.

- Quote arguments to 'eval' to prevent word-splitting.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 examples/cbq.init-v0.7.3 | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/examples/cbq.init-v0.7.3 b/examples/cbq.init-v0.7.3
index 1bc0d446f8983..66448d88f0053 100644
--- a/examples/cbq.init-v0.7.3
+++ b/examples/cbq.init-v0.7.3
@@ -532,7 +532,7 @@ cbq_off () {
 
 ### Prefixed message
 cbq_message () {
-	echo -e "**CBQ: $@"
+	echo -e "**CBQ: $*"
 } # cbq_message
 
 ### Failure message
@@ -560,15 +560,15 @@ cbq_time2abs () {
 ### Display CBQ setup
 cbq_show () {
 	for dev in `cbq_device_list`; do
-		[ `tc qdisc show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc qdisc show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: queueing disciplines\n"
 		tc $1 qdisc show dev $dev; echo
 
-		[ `tc class show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc class show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: traffic classes\n"
 		tc $1 class show dev $dev; echo
 
-		[ `tc filter show dev $dev| wc -l` -eq 0 ] && continue
+		[ "`tc filter show dev $dev| wc -l`" -eq 0 ] && continue
 		echo -e "### $dev: filtering rules\n"
 		tc $1 filter show dev $dev; echo
 	done
@@ -585,7 +585,7 @@ cbq_init () {
 
 	### Gather all DEVICE fields from $1/cbq-*
 	DEVFIELDS=`find $1 -maxdepth 1 \( -type f -or -type l \) -name 'cbq-*' \
-		  -not -name '*~' | xargs sed -n 's/#.*//; \
+		  -not -name '*~' -print0 | xargs -0 sed -n 's/#.*//; \
 		  s/[[:space:]]//g; /^DEVICE=[^,]*,[^,]*\(,[^,]*\)\?/ \
 		  { s/.*=//; p; }'| sort -u`
 	[ -z "$DEVFIELDS" ] &&
@@ -593,7 +593,7 @@ cbq_init () {
 
 	### Check for different DEVICE fields for the same device
 	DEVICES=`echo "$DEVFIELDS"| sed 's/,.*//'| sort -u`
-	[ `echo "$DEVICES"| wc -l` -ne `echo "$DEVFIELDS"| wc -l` ] &&
+	[ "`echo "$DEVICES"| wc -l`" -ne "`echo "$DEVFIELDS"| wc -l`" ] &&
 		cbq_failure "different DEVICE fields for single device!\n$DEVFIELDS"
 } # cbq_init
 
@@ -618,7 +618,7 @@ cbq_load_class () {
 	PRIO_MARK=$PRIO_MARK_DEFAULT
 	PRIO_REALM=$PRIO_REALM_DEFAULT
 
-	eval `echo "$CFILE"| grep -E "^($CBQ_WORDS)="`
+	eval "`echo "$CFILE"| grep -E "^($CBQ_WORDS)="`"
 
 	### Require RATE/WEIGHT
 	[ -z "$RATE" -o -z "$WEIGHT" ] &&
@@ -661,7 +661,7 @@ if [ "$1" = "compile" ]; then
 
 	### echo-only version of "tc" command
 	tc () {
-		echo "$TC $@"
+		echo "$TC $*"
 	} # tc
 
 elif [ -n "$CBQ_DEBUG" ]; then
@@ -669,13 +669,13 @@ elif [ -n "$CBQ_DEBUG" ]; then
 
 	### Logging version of "ip" command
 	ip () {
-		echo -e "\n# ip $@" >> $CBQ_DEBUG
+		echo -e "\n# ip $*" >> $CBQ_DEBUG
 		$IP "$@" 2>&1 | tee -a $CBQ_DEBUG
 	} # ip
 
 	### Logging version of "tc" command
 	tc () {
-		echo -e "\n# tc $@" >> $CBQ_DEBUG
+		echo -e "\n# tc $*" >> $CBQ_DEBUG
 		$TC "$@" 2>&1 | tee -a $CBQ_DEBUG
 	} # tc
 else
@@ -711,8 +711,8 @@ if [ "$1" != "compile" -a "$2" != "nocache" -a -z "$CBQ_DEBUG" ]; then
 	### validate the cache
 	[ "$2" = "invalidate" -o ! -f $CBQ_CACHE ] && VALID=0
 	if [ $VALID -eq 1 ]; then
-		[ `find $CBQ_PATH -maxdepth 1 -newer $CBQ_CACHE| \
-		  wc -l` -gt 0 ] && VALID=0
+		[ "`find $CBQ_PATH -maxdepth 1 -newer $CBQ_CACHE| \
+		  wc -l`" -gt 0 ] && VALID=0
 	fi
 
 	### compile the config if the cache is invalid
-- 
2.13.1

[iproute PATCH v2 2/2] ifcfg: Quote left-hand side of [ ] expression

[Phil Sutter]

This prevents word-splitting and therefore leads to more accurate error
message in case 'grep -c' prints something other than a number.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ifcfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ifcfg b/ip/ifcfg
index 083d9df36742f..30a2dc49816cd 100644
--- a/ip/ifcfg
+++ b/ip/ifcfg
@@ -131,7 +131,7 @@ noarp=$?
 
 ip route add unreachable 224.0.0.0/24 >& /dev/null
 ip route add unreachable 255.255.255.255 >& /dev/null
-if [ `ip link ls $dev | grep -c MULTICAST` -ge 1 ]; then
+if [ "`ip link ls $dev | grep -c MULTICAST`" -ge 1 ]; then
   ip route add 224.0.0.0/4 dev $dev scope global >& /dev/null
 fi
 
-- 
2.13.1

RE: [iproute PATCH v2 2/2] ifcfg: Quote left-hand side of [ ] expression

[David Laight]

From: Phil Sutter
> Sent: 17 August 2017 18:10
> This prevents word-splitting and therefore leads to more accurate error
> message in case 'grep -c' prints something other than a number.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/ifcfg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ip/ifcfg b/ip/ifcfg
> index 083d9df36742f..30a2dc49816cd 100644
> --- a/ip/ifcfg
> +++ b/ip/ifcfg
> @@ -131,7 +131,7 @@ noarp=$?
> 
>  ip route add unreachable 224.0.0.0/24 >& /dev/null
>  ip route add unreachable 255.255.255.255 >& /dev/null
> -if [ `ip link ls $dev | grep -c MULTICAST` -ge 1 ]; then
> +if [ "`ip link ls $dev | grep -c MULTICAST`" -ge 1 ]; then

You could drag all these scripts into the 1990's by using $(...)
instead of `...`.

	David

Re: [iproute PATCH v2 2/2] ifcfg: Quote left-hand side of [ ] expression

[Phil Sutter]

On Fri, Aug 18, 2017 at 09:32:52AM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 17 August 2017 18:10
> > This prevents word-splitting and therefore leads to more accurate error
> > message in case 'grep -c' prints something other than a number.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  ip/ifcfg | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/ip/ifcfg b/ip/ifcfg
> > index 083d9df36742f..30a2dc49816cd 100644
> > --- a/ip/ifcfg
> > +++ b/ip/ifcfg
> > @@ -131,7 +131,7 @@ noarp=$?
> > 
> >  ip route add unreachable 224.0.0.0/24 >& /dev/null
> >  ip route add unreachable 255.255.255.255 >& /dev/null
> > -if [ `ip link ls $dev | grep -c MULTICAST` -ge 1 ]; then
> > +if [ "`ip link ls $dev | grep -c MULTICAST`" -ge 1 ]; then
> 
> You could drag all these scripts into the 1990's by using $(...)
> instead of `...`.

That's a different kettle of fish IMO, using $() doesn't change the
situation this addresses:

| $ [ $(echo foo bar) -eq 0 ] && echo ok
| bash: [: too many arguments
| $ [ "$(echo foo bar)" -eq 0 ] && echo ok
| bash: [: foo bar: integer expression expected

Cheers, Phil

Re: [iproute PATCH v2 0/2] Covscan: Shell script fixes

[Stephen Hemminger]

On Thu, 17 Aug 2017 19:09:30 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This series collects patches from v1 which deal with programming
> mistakes in shell scripts.
> 
> No changes to the actual patches, just splitting into smaller series.
> 
> Phil Sutter (2):
>   examples: Some shell fixes to cbq.init
>   ifcfg: Quote left-hand side of [ ] expression
> 
>  examples/cbq.init-v0.7.3 | 24 ++++++++++++------------
>  ip/ifcfg                 |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 

Applied.

RE: [iproute PATCH v2 2/2] ifcfg: Quote left-hand side of [ ] expression

[David Laight]

From: Phil Sutter
> Sent: 18 August 2017 12:24
...
> > > -if [ `ip link ls $dev | grep -c MULTICAST` -ge 1 ]; then
> > > +if [ "`ip link ls $dev | grep -c MULTICAST`" -ge 1 ]; then
> >
> > You could drag all these scripts into the 1990's by using $(...)
> > instead of `...`.
> 
> That's a different kettle of fish IMO, using $() doesn't change the
> situation this addresses:
> 
> | $ [ $(echo foo bar) -eq 0 ] && echo ok
> | bash: [: too many arguments
> | $ [ "$(echo foo bar)" -eq 0 ] && echo ok
> | bash: [: foo bar: integer expression expected

I didn't say it would.
IFS=
set -o noglob
would though - not that I'd suggest it here.
protecting the 'eval' is somewhat harder.

	David