| Message ID | 20241016052042.1138320-1-lizhijian@fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ndctl,v2] test/monitor.sh: Fix 2 bash syntax errors | expand |
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"
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 > >
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 >> >>
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
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"
$ 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(-)