diff mbox series

[ndctl,v2,2/2] test/cxl-region-sysfs.sh: Fix cxl-region-sysfs.sh: line 107: [: missing `]'

Message ID 20231212074228.1261164-2-lizhijian@fujitsu.com
State Superseded
Headers show
Series [ndctl,v2,1/2] test/cxl-region-sysfs.sh: use '[[ ]]' command to evaluate operands as arithmetic expressions | expand

Commit Message

Li Zhijian Dec. 12, 2023, 7:42 a.m. UTC
A space is missing before ']'

Acked-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 test/cxl-region-sysfs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alison Schofield Dec. 12, 2023, 8:54 p.m. UTC | #1
On Tue, Dec 12, 2023 at 03:42:28PM +0800, Li Zhijian wrote:
> A space is missing before ']'

What's happens when that space is missing?
That's partly a request to add an impact statement, but also
for my education as I'm just learning all this shellcheck
stuff too.

BTW - if any of this was found using a tool, rather than
by inspection, please include a note of the tool used.

Thanks,
Alison

> 
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  test/cxl-region-sysfs.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
> index 6a5da6d..db1a163 100644
> --- a/test/cxl-region-sysfs.sh
> +++ b/test/cxl-region-sysfs.sh
> @@ -104,7 +104,7 @@ do
>  	iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways)
>  	ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity)
>  	[ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets"
> -	[ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
> +	[ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
>  
>  	sz=$(cat /sys/bus/cxl/devices/$i/size)
>  	res=$(cat /sys/bus/cxl/devices/$i/start)
> -- 
> 2.41.0
> 
>
Dan Williams Dec. 12, 2023, 10:46 p.m. UTC | #2
Li Zhijian wrote:
> A space is missing before ']'
> 
> Acked-by: Dan Williams <dan.j.williams@intel.com>

You added my Ack without addressing the feedback.

"Commentary on the impact of the change is always welcome."

Please include a sentence on how this triggered for you and some
analysis of the why it has not triggered previously. Otherwise "A space
is missing before ']'" does not add any information that can not be
determined by reading the patch.

A useful changelog for this would be something like:

Currently the cxl-region-sysfs.sh test runs to completion and passes,
but with syntax errors in the log. It turns out that because the test is
checking for a positive condition as a failure, that also happens to
mask the syntax errors. Fix the syntax and note that this also happens
to unblock a test case that was being hidden by this error.
Li Zhijian Dec. 13, 2023, 7:50 a.m. UTC | #3
On 13/12/2023 06:46, Dan Williams wrote:
> Li Zhijian wrote:
>> A space is missing before ']'
>>
>> Acked-by: Dan Williams <dan.j.williams@intel.com>
> 
> You added my Ack without addressing the feedback.

I'm sorry about that. I thought this fix(syntax error) is too simple to say more.


> 
> "Commentary on the impact of the change is always welcome."
> 
> Please include a sentence on how this triggered for you and some
> analysis of the why it has not triggered previously. Otherwise "A space
> is missing before ']'" does not add any information that can not be
> determined by reading the patch.
> 
> A useful changelog for this would be something like:
> > Currently the cxl-region-sysfs.sh test runs to completion and passes,
> but with syntax errors in the log. It turns out that because the test is
> checking for a positive condition as a failure, that also happens to
> mask the syntax errors. Fix the syntax and note that this also happens
> to unblock a test case that was being hidden by this error.


thanks, it's a pretty good changelog.

will update in V3.


Thanks
Zhijian
Li Zhijian Dec. 13, 2023, 8 a.m. UTC | #4
On 13/12/2023 04:54, Alison Schofield wrote:
> On Tue, Dec 12, 2023 at 03:42:28PM +0800, Li Zhijian wrote:
>> A space is missing before ']'
> 
> What's happens when that space is missing?

It's a syntax error, so
'[ $ig -ne $r_ig]' is always false, it may hide the a real error.



> That's partly a request to add an impact statement, but also
> for my education as I'm just learning all this shellcheck
> stuff too.

shellcheck is able to inspect this error.

> 
> BTW - if any of this was found using a tool, rather than
> by inspection, please include a note of the tool used.

This time, they are all found by eyes :)
when i'm checking the full log when i met an error.


Thanks
Zhijian

> 
> Thanks,
> Alison
> 
>>
>> Acked-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   test/cxl-region-sysfs.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
>> index 6a5da6d..db1a163 100644
>> --- a/test/cxl-region-sysfs.sh
>> +++ b/test/cxl-region-sysfs.sh
>> @@ -104,7 +104,7 @@ do
>>   	iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways)
>>   	ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity)
>>   	[ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets"
>> -	[ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
>> +	[ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
>>   
>>   	sz=$(cat /sys/bus/cxl/devices/$i/size)
>>   	res=$(cat /sys/bus/cxl/devices/$i/start)
>> -- 
>> 2.41.0
>>
>>
>
diff mbox series

Patch

diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
index 6a5da6d..db1a163 100644
--- a/test/cxl-region-sysfs.sh
+++ b/test/cxl-region-sysfs.sh
@@ -104,7 +104,7 @@  do
 	iw=$(cat /sys/bus/cxl/devices/$i/interleave_ways)
 	ig=$(cat /sys/bus/cxl/devices/$i/interleave_granularity)
 	[ $iw -ne $nr_targets ] && err "$LINENO: decoder: $i iw: $iw targets: $nr_targets"
-	[ $ig -ne $r_ig] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
+	[ $ig -ne $r_ig ] && err "$LINENO: decoder: $i ig: $ig root ig: $r_ig"
 
 	sz=$(cat /sys/bus/cxl/devices/$i/size)
 	res=$(cat /sys/bus/cxl/devices/$i/start)