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

[Alison Schofield <alison.schofield@intel.com>]

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
> 
>