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

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

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