kernel/fs/doio/rwtest: fix shellcheck error

Message ID 20180525074447.26298-1-yixin.zhang@intel.com
State Rejected
Delegated to: Petr Vorel
Headers show
Series
  • kernel/fs/doio/rwtest: fix shellcheck error
Related show

Commit Message

Zhang, Yixin May 25, 2018, 7:44 a.m.
testcases/kernel/fs/doio/rwtest:199:12: error: > is for string comparisons. Use -gt instead. [SC2071]
testcases/kernel/fs/doio/rwtest:320:10: error: Globs are ignored in [[ ]] except right of =/!=. Use a loop. [SC2203]
testcases/kernel/fs/doio/rwtest:347:7: error: Couldn't parse this test expression. [SC1073]
testcases/kernel/fs/doio/rwtest:347:25: error: Expected test to end here (don't wrap commands in []/[[]]).
                                               Fix any mentioned problems and try again. [SC1072]

Signed-off-by: Yixin Zhang <yixin.zhang@intel.com>
---
 testcases/kernel/fs/doio/rwtest | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Petr Vorel June 25, 2018, 8:41 a.m. | #1
Hi Yixin,

thanks for your patch, we appreciate your effort.
There are some comments bellow. Even if you fix them, patches like this one
doesn't help much. The script is really ugly, requires a rewrite which would:

1) Be posix compliant. I.e.: remove all bashisms (arrays, typeset, regex
in [[ ... ]], etc.). It's good to use checkbashism.pl script and run it in dash
to test it:
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style
https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl

2) Use LTP new shell API
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell

Rewriting the script would be a great improvement. If you decide to do so,
please do not hesitate to ask for help (if needed).

Kind regards,
Petr

> testcases/kernel/fs/doio/rwtest:199:12: error: > is for string comparisons. Use -gt instead. [SC2071]
> testcases/kernel/fs/doio/rwtest:320:10: error: Globs are ignored in [[ ]] except right of =/!=. Use a loop. [SC2203]
> testcases/kernel/fs/doio/rwtest:347:7: error: Couldn't parse this test expression. [SC1073]
> testcases/kernel/fs/doio/rwtest:347:25: error: Expected test to end here (don't wrap commands in []/[[]]).
>                                                Fix any mentioned problems and try again. [SC1072]

...
> +++ b/testcases/kernel/fs/doio/rwtest
> @@ -196,7 +196,7 @@ do	case $1 in
>  	-n | -Dn )
>  		dOpts="$dOpts $1 $2"
>  		# force file locking with > 1 process
> -		if [[ $2 > 1 ]]
> +		if [[ $2 -gt 1 ]]
Why not use posix compliant syntax? I.e. single square brackets:
if [ $2 -gt 1 ]; then

>  		then
>  			dOpts="$dOpts -k"
>  		fi
> @@ -317,7 +317,7 @@ do
>  		typeset -i n=0
>  		while (( n < ${#szcache[*]} ))
>  		do
> -			if [[ szcache[$n] = $dir ]]; then
> +			if [[ ${szcache[n]} = $dir ]]; then
Well, correct, but we'd like to drop arrays, as they're bashisms.
>  				break;
>  			fi
>  			n=n+1
> @@ -344,7 +344,7 @@ do

>  			# check if blks is a number, else set a default value for blks
>  			default_sz=1000000
> -			if [ $blks -eq $blks 2> /dev/null ]
> +			if [ $blks -eq $blks ] 2>/dev/null
The shellcheck warning was meant to be without square brackets:
if $blks -eq $blks 2>/dev/null; then
Zhang, Yixin July 6, 2018, 7:14 a.m. | #2
Hi Petr,

Sorry for late reply, I was busy on something else in the past weeks.
Thanks for all the comments, I'll provide a re-write version soon.

Yixin

On 2018-06-25 at 10:41:20 +0200, Petr Vorel wrote:
> Hi Yixin,
> 
> thanks for your patch, we appreciate your effort.
> There are some comments bellow. Even if you fix them, patches like this one
> doesn't help much. The script is really ugly, requires a rewrite which would:
> 
> 1) Be posix compliant. I.e.: remove all bashisms (arrays, typeset, regex
> in [[ ... ]], etc.). It's good to use checkbashism.pl script and run it in dash
> to test it:
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style
> https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl
> 
> 2) Use LTP new shell API
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
> 
> Rewriting the script would be a great improvement. If you decide to do so,
> please do not hesitate to ask for help (if needed).
> 
> Kind regards,
> Petr
> 
> > testcases/kernel/fs/doio/rwtest:199:12: error: > is for string comparisons. Use -gt instead. [SC2071]
> > testcases/kernel/fs/doio/rwtest:320:10: error: Globs are ignored in [[ ]] except right of =/!=. Use a loop. [SC2203]
> > testcases/kernel/fs/doio/rwtest:347:7: error: Couldn't parse this test expression. [SC1073]
> > testcases/kernel/fs/doio/rwtest:347:25: error: Expected test to end here (don't wrap commands in []/[[]]).
> >                                                Fix any mentioned problems and try again. [SC1072]
> 
> ...
> > +++ b/testcases/kernel/fs/doio/rwtest
> > @@ -196,7 +196,7 @@ do	case $1 in
> >  	-n | -Dn )
> >  		dOpts="$dOpts $1 $2"
> >  		# force file locking with > 1 process
> > -		if [[ $2 > 1 ]]
> > +		if [[ $2 -gt 1 ]]
> Why not use posix compliant syntax? I.e. single square brackets:
> if [ $2 -gt 1 ]; then
> 
> >  		then
> >  			dOpts="$dOpts -k"
> >  		fi
> > @@ -317,7 +317,7 @@ do
> >  		typeset -i n=0
> >  		while (( n < ${#szcache[*]} ))
> >  		do
> > -			if [[ szcache[$n] = $dir ]]; then
> > +			if [[ ${szcache[n]} = $dir ]]; then
> Well, correct, but we'd like to drop arrays, as they're bashisms.
> >  				break;
> >  			fi
> >  			n=n+1
> > @@ -344,7 +344,7 @@ do
> 
> >  			# check if blks is a number, else set a default value for blks
> >  			default_sz=1000000
> > -			if [ $blks -eq $blks 2> /dev/null ]
> > +			if [ $blks -eq $blks ] 2>/dev/null
> The shellcheck warning was meant to be without square brackets:
> if $blks -eq $blks 2>/dev/null; then
>

Patch

diff --git a/testcases/kernel/fs/doio/rwtest b/testcases/kernel/fs/doio/rwtest
index 90b1658f5..bd6bf9de2 100644
--- a/testcases/kernel/fs/doio/rwtest
+++ b/testcases/kernel/fs/doio/rwtest
@@ -196,7 +196,7 @@  do	case $1 in
 	-n | -Dn )
 		dOpts="$dOpts $1 $2"
 		# force file locking with > 1 process
-		if [[ $2 > 1 ]]
+		if [[ $2 -gt 1 ]]
 		then
 			dOpts="$dOpts -k"
 		fi
@@ -317,7 +317,7 @@  do
 		typeset -i n=0
 		while (( n < ${#szcache[*]} ))
 		do
-			if [[ szcache[$n] = $dir ]]; then
+			if [[ ${szcache[n]} = $dir ]]; then
 				break;
 			fi
 			n=n+1
@@ -344,7 +344,7 @@  do
 
 			# check if blks is a number, else set a default value for blks
 			default_sz=1000000
-			if [ $blks -eq $blks 2> /dev/null ]
+			if [ $blks -eq $blks ] 2>/dev/null
 			then
 
 				case $(uname) in