[ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[Li Zhijian]

$ grep -w line build/meson-logs/testlog.txt
test/monitor.sh: line 99: [: too many arguments
test/monitor.sh: line 99: [: nmem0: binary operator expected
test/monitor.sh: line 149: 40.0: syntax error: invalid arithmetic operator (error token is ".0")

- monitor_dimms could be a string with multiple *spaces*, like: "nmem0 nmem1 nmem2"
- inject_value is a float value, like 40.0, which need to be converted to
  integer before operation: $((inject_value + 1))

Some features have not been really verified due to these errors

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V1:
 V1 has a mistake which overts to integer too late.
 Move the conversion forward before the operation
---
 test/monitor.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/monitor.sh b/test/monitor.sh
index c5beb2c..7809a7c 100755
--- a/test/monitor.sh
+++ b/test/monitor.sh
@@ -96,7 +96,7 @@ test_filter_region()
 	while [ $i -lt $count ]; do
 		monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r .[$i].dev)
 		monitor_dimms=$(get_monitor_dimm "-r $monitor_region")
-		[ ! -z $monitor_dimms ] && break
+		[ ! -z "$monitor_dimms" ] && break
 		i=$((i + 1))
 	done
 	start_monitor "-r $monitor_region"
@@ -146,6 +146,7 @@ test_filter_dimmevent()
 	stop_monitor
 
 	inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r .[]."health"."temperature_threshold")
+	inject_value=${inject_value%.*}
 	inject_value=$((inject_value + 1))
 	start_monitor "-d $monitor_dimms -D dimm-media-temperature"
 	inject_smart "-m $inject_value"
-- 
2.44.0

Re: [ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[Verma, Vishal L]

On Wed, 2024-10-16 at 13:20 +0800, Li Zhijian wrote:
> $ grep -w line build/meson-logs/testlog.txt
> test/monitor.sh: line 99: [: too many arguments
> test/monitor.sh: line 99: [: nmem0: binary operator expected
> test/monitor.sh: line 149: 40.0: syntax error: invalid arithmetic operator (error token is ".0")
> 
> - monitor_dimms could be a string with multiple *spaces*, like: "nmem0 nmem1 nmem2"
> - inject_value is a float value, like 40.0, which need to be converted to
>   integer before operation: $((inject_value + 1))
> 
> Some features have not been really verified due to these errors
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V1:
>  V1 has a mistake which overts to integer too late.
>  Move the conversion forward before the operation
> ---
>  test/monitor.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/test/monitor.sh b/test/monitor.sh
> index c5beb2c..7809a7c 100755
> --- a/test/monitor.sh
> +++ b/test/monitor.sh
> @@ -96,7 +96,7 @@ test_filter_region()
>  	while [ $i -lt $count ]; do
>  		monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r .[$i].dev)
>  		monitor_dimms=$(get_monitor_dimm "-r $monitor_region")
> -		[ ! -z $monitor_dimms ] && break
> +		[ ! -z "$monitor_dimms" ] && break

[ ! -z "..." ] is a bit of a double negative, while we are changing
this, I'd suggest cleaning up a bit more such as:

   if [[ "$monitor_dimms" ]]; then
   	break
   fi

Other than that looks good,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

>  		i=$((i + 1))
>  	done
>  	start_monitor "-r $monitor_region"
> @@ -146,6 +146,7 @@ test_filter_dimmevent()
>  	stop_monitor
>  
>  	inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r .[]."health"."temperature_threshold")
> +	inject_value=${inject_value%.*}
>  	inject_value=$((inject_value + 1))
>  	start_monitor "-d $monitor_dimms -D dimm-media-temperature"
>  	inject_smart "-m $inject_value"

Re: [ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[Alison Schofield]

On Wed, Oct 16, 2024 at 10:45:48PM +0000, Vishal Verma wrote:
> On Wed, 2024-10-16 at 13:20 +0800, Li Zhijian wrote:
> > $ grep -w line build/meson-logs/testlog.txt
> > test/monitor.sh: line 99: [: too many arguments
> > test/monitor.sh: line 99: [: nmem0: binary operator expected
> > test/monitor.sh: line 149: 40.0: syntax error: invalid arithmetic operator (error token is ".0")
> > 
> > - monitor_dimms could be a string with multiple *spaces*, like: "nmem0 nmem1 nmem2"
> > - inject_value is a float value, like 40.0, which need to be converted to
> >   integer before operation: $((inject_value + 1))
> > 
> > Some features have not been really verified due to these errors
> > 
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> > ---
> > V1:
> >  V1 has a mistake which overts to integer too late.
> >  Move the conversion forward before the operation
> > ---
> >  test/monitor.sh | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/test/monitor.sh b/test/monitor.sh
> > index c5beb2c..7809a7c 100755
> > --- a/test/monitor.sh
> > +++ b/test/monitor.sh
> > @@ -96,7 +96,7 @@ test_filter_region()
> >  	while [ $i -lt $count ]; do
> >  		monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r .[$i].dev)
> >  		monitor_dimms=$(get_monitor_dimm "-r $monitor_region")
> > -		[ ! -z $monitor_dimms ] && break
> > +		[ ! -z "$monitor_dimms" ] && break
> 
> [ ! -z "..." ] is a bit of a double negative, while we are changing
> this, I'd suggest cleaning up a bit more such as:
> 
>    if [[ "$monitor_dimms" ]]; then
>    	break
>    fi
> 
> Other than that looks good,
> 
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

BTW - I second Vishal's suggestion here.
Shellcheck is catching bad syntax but may not be suggesting the
best syntax as an alternative. 

--Alison

snip

Re: [ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[Alison Schofield]

On Wed, Oct 16, 2024 at 01:20:42PM +0800, Li Zhijian wrote:
> $ grep -w line build/meson-logs/testlog.txt
> test/monitor.sh: line 99: [: too many arguments
> test/monitor.sh: line 99: [: nmem0: binary operator expected
> test/monitor.sh: line 149: 40.0: syntax error: invalid arithmetic operator (error token is ".0")
> 
> - monitor_dimms could be a string with multiple *spaces*, like: "nmem0 nmem1 nmem2"
> - inject_value is a float value, like 40.0, which need to be converted to
>   integer before operation: $((inject_value + 1))
> 
> Some features have not been really verified due to these errors
> 

Good eye finding the complaints of a passing unit test!
I'm confused on why the trap on err spewed the line number but
didn't fail the test. 

While I, maybe others chew on that, thanks for the patch and
can you do more :)  ?

By quoting $monitor_dimms in the -z check ([ ! -z "$monitor_dimms" ]),
the script breaks out of the loop correctly and monitors the first region
that has DIMMs. I don't know if the test cared if it got the first or
second region of nfit_test, but the syntax is definately wrong, and it's
error prone in mulitple places in that shell script.

'$ shellcheck monitor.sh' shows the instance you found:

>> In monitor.sh line 99:
>> 		[ ! -z $monitor_dimms ] && break
>>                   ^-- SC2236: Use -n instead of ! -z.
>>                        ^------------^ SC2086: Double quote to prevent globbing and word splitting.
>> 
>> Did you mean: 
>> 		[ ! -z "$monitor_dimms" ] && break
>> 

There are a bunch more instances in need of double quotes. Can you turn
this around as a new patch that cleans it all. Note that you might not
be able to get rid of all shellcheck complaints in the boilerplate of
the script, but should be able to clean up the main body of the script
and prevent more problems like you found here.

I'll suggest turning this patch into a 2 patches:

1/1: test/monitor.sh address shellcheck bash syntax issues
2/2: test/monitor.sh convert float to integer before increment

For 2/2 it does stop the test prematurely. We never run the temperature
inject test case of test_filter_dimmevent() because of the inability
to increment the float. Please include that impact statement in the
commit log.

-- Alison

> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V1:
>  V1 has a mistake which overts to integer too late.
>  Move the conversion forward before the operation
> ---
>  test/monitor.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/test/monitor.sh b/test/monitor.sh
> index c5beb2c..7809a7c 100755
> --- a/test/monitor.sh
> +++ b/test/monitor.sh
> @@ -96,7 +96,7 @@ test_filter_region()
>  	while [ $i -lt $count ]; do
>  		monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r .[$i].dev)
>  		monitor_dimms=$(get_monitor_dimm "-r $monitor_region")
> -		[ ! -z $monitor_dimms ] && break
> +		[ ! -z "$monitor_dimms" ] && break
>  		i=$((i + 1))
>  	done
>  	start_monitor "-r $monitor_region"
> @@ -146,6 +146,7 @@ test_filter_dimmevent()
>  	stop_monitor
>  
>  	inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r .[]."health"."temperature_threshold")
> +	inject_value=${inject_value%.*}
>  	inject_value=$((inject_value + 1))
>  	start_monitor "-d $monitor_dimms -D dimm-media-temperature"
>  	inject_smart "-m $inject_value"
> -- 
> 2.44.0
> 
> 

Re: [ndctl PATCH v2] test/monitor.sh: Fix 2 bash syntax errors

[Zhijian Li (Fujitsu)]

On 17/10/2024 09:00, Alison Schofield wrote:
> On Wed, Oct 16, 2024 at 01:20:42PM +0800, Li Zhijian wrote:
>> $ grep -w line build/meson-logs/testlog.txt
>> test/monitor.sh: line 99: [: too many arguments
>> test/monitor.sh: line 99: [: nmem0: binary operator expected
>> test/monitor.sh: line 149: 40.0: syntax error: invalid arithmetic operator (error token is ".0")
>>
>> - monitor_dimms could be a string with multiple *spaces*, like: "nmem0 nmem1 nmem2"
>> - inject_value is a float value, like 40.0, which need to be converted to
>>    integer before operation: $((inject_value + 1))
>>
>> Some features have not been really verified due to these errors
>>
> 
> Good eye finding the complaints of a passing unit test!
> I'm confused on why the trap on err spewed the line number but
> didn't fail the test.
> 
> While I, maybe others chew on that, thanks for the patch and
> can you do more :)  ?

Sure, I will do that later.


> 
> By quoting $monitor_dimms in the -z check ([ ! -z "$monitor_dimms" ]),
> the script breaks out of the loop correctly and monitors the first region
> that has DIMMs. I don't know if the test cared if it got the first or
> second region of nfit_test, but the syntax is definately wrong, and it's
> error prone in mulitple places in that shell script.
> 
> '$ shellcheck monitor.sh' shows the instance you found:
> 
>>> In monitor.sh line 99:
>>> 		[ ! -z $monitor_dimms ] && break
>>>                    ^-- SC2236: Use -n instead of ! -z.
>>>                         ^------------^ SC2086: Double quote to prevent globbing and word splitting.
>>>
>>> Did you mean:
>>> 		[ ! -z "$monitor_dimms" ] && break
>>>
> 
> There are a bunch more instances in need of double quotes. Can you turn
> this around as a new patch that cleans it all. Note that you might not
> be able to get rid of all shellcheck complaints in the boilerplate of
> the script, but should be able to clean up the main body of the script
> and prevent more problems like you found here.
> 
> I'll suggest turning this patch into a 2 patches:
> 
> 1/1: test/monitor.sh address shellcheck bash syntax issues
> 2/2: test/monitor.sh convert float to integer before increment

Sound good to me


> 
> For 2/2 it does stop the test prematurely. We never run the temperature
> inject test case of test_filter_dimmevent() because of the inability
> to increment the float. Please include that impact statement in the
> commit log.

Sure


Thanks
Zhijian

> 
> -- Alison
> 
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> V1:
>>   V1 has a mistake which overts to integer too late.
>>   Move the conversion forward before the operation
>> ---
>>   test/monitor.sh | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/monitor.sh b/test/monitor.sh
>> index c5beb2c..7809a7c 100755
>> --- a/test/monitor.sh
>> +++ b/test/monitor.sh
>> @@ -96,7 +96,7 @@ test_filter_region()
>>   	while [ $i -lt $count ]; do
>>   		monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r .[$i].dev)
>>   		monitor_dimms=$(get_monitor_dimm "-r $monitor_region")
>> -		[ ! -z $monitor_dimms ] && break
>> +		[ ! -z "$monitor_dimms" ] && break
>>   		i=$((i + 1))
>>   	done
>>   	start_monitor "-r $monitor_region"
>> @@ -146,6 +146,7 @@ test_filter_dimmevent()
>>   	stop_monitor
>>   
>>   	inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r .[]."health"."temperature_threshold")
>> +	inject_value=${inject_value%.*}
>>   	inject_value=$((inject_value + 1))
>>   	start_monitor "-d $monitor_dimms -D dimm-media-temperature"
>>   	inject_smart "-m $inject_value"
>> -- 
>> 2.44.0
>>
>>