[ndctl PATCH 1/3] test/cxl-region-sysfs.sh: covert size and resource to hex before test

[ndctl PATCH 1/3] test/cxl-region-sysfs.sh: covert size and resource to hex before test

[Li Zhijian]

size and resource are both decimal

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 test/cxl-region-sysfs.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
index 8636392..ded7aa1 100644
--- a/test/cxl-region-sysfs.sh
+++ b/test/cxl-region-sysfs.sh
@@ -123,6 +123,11 @@ readarray -t switch_decoders < <(echo $json | jq -r ".[].decoder")
 [ ${#switch_decoders[@]} -ne $nr_switch_decoders ] && err \
 "$LINENO: expected $nr_switch_decoders got ${#switch_decoders[@]} switch decoders"
 
+decimal_to_hex()
+{
+	printf "0x%x" $1
+}
+
 for i in ${switch_decoders[@]}
 do
 	decoder=$(echo $json | jq -r ".[] | select(.decoder == \"$i\")")
@@ -136,8 +141,8 @@ do
 	[ $ig -ne $((r_ig << depth)) ] && err \
 	"$LINENO: decoder: $i ig: $ig switch_ig: $((r_ig << depth))"
 
-	res=$(echo $decoder | jq -r ".resource")
-	sz=$(echo $decoder | jq -r ".size")
+	res=$(decimal_to_hex $(echo $decoder | jq -r ".resource"))
+	sz=$(decimal_to_hex $(echo $decoder | jq -r ".size"))
 	[ $sz -ne $region_size ] && err \
 	"$LINENO: decoder: $i sz: $sz region_size: $region_size"
 	[ $res -ne $region_base ] && err \
-- 
2.41.0

[ndctl PATCH 2/3] test/cxl-region-sysfs.sh: use operator '!=' to compare hexadecimal value

[Li Zhijian]

Fix errors:
line 111: [: 0x80000000: integer expression expected
line 112: [: 0x3ff110000000: integer expression expected
line 141: [: 0x80000000: integer expression expected
line 143: [: 0x3ff110000000: integer expression expected

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 test/cxl-region-sysfs.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
index ded7aa1..89f21a3 100644
--- a/test/cxl-region-sysfs.sh
+++ b/test/cxl-region-sysfs.sh
@@ -108,8 +108,8 @@ do
 
 	sz=$(cat /sys/bus/cxl/devices/$i/size)
 	res=$(cat /sys/bus/cxl/devices/$i/start)
-	[ $sz -ne $region_size ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
-	[ $res -ne $region_base ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"
+	[ "$sz" != "$region_size" ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
+	[ "$res" != "$region_base" ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"
 done
 
 # validate all switch decoders have the correct settings
@@ -143,9 +143,9 @@ do
 
 	res=$(decimal_to_hex $(echo $decoder | jq -r ".resource"))
 	sz=$(decimal_to_hex $(echo $decoder | jq -r ".size"))
-	[ $sz -ne $region_size ] && err \
+	[ "$sz" != "$region_size" ] && err \
 	"$LINENO: decoder: $i sz: $sz region_size: $region_size"
-	[ $res -ne $region_base ] && err \
+	[ "$res" != "$region_base" ] && err \
 	"$LINENO: decoder: $i base: $res region_base: $region_base"
 done
 
-- 
2.41.0

[ndctl PATCH 3/3] test/cxl-region-sysfs.sh: Fix cxl-region-sysfs.sh: line 107: [: missing `]'

[Li Zhijian]

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 89f21a3..3878351 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

RE: [ndctl PATCH 1/3] test/cxl-region-sysfs.sh: covert size and resource to hex before test

[Dan Williams]

I would summarize this as "Fix hex vs decimal confusion"

Li Zhijian wrote:
> size and resource are both decimal

Please always describe the "why" in the changelog including the
motivation for the change and the effect of not applying the patch. I.e.
how would someone know that they are hitting the problem that this patch
fixes. Sample output usually helps here.

> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  test/cxl-region-sysfs.sh | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
> index 8636392..ded7aa1 100644
> --- a/test/cxl-region-sysfs.sh
> +++ b/test/cxl-region-sysfs.sh
> @@ -123,6 +123,11 @@ readarray -t switch_decoders < <(echo $json | jq -r ".[].decoder")
>  [ ${#switch_decoders[@]} -ne $nr_switch_decoders ] && err \
>  "$LINENO: expected $nr_switch_decoders got ${#switch_decoders[@]} switch decoders"
>  
> +decimal_to_hex()
> +{
> +	printf "0x%x" $1
> +}
> +
>  for i in ${switch_decoders[@]}
>  do
>  	decoder=$(echo $json | jq -r ".[] | select(.decoder == \"$i\")")
> @@ -136,8 +141,8 @@ do
>  	[ $ig -ne $((r_ig << depth)) ] && err \
>  	"$LINENO: decoder: $i ig: $ig switch_ig: $((r_ig << depth))"
>  
> -	res=$(echo $decoder | jq -r ".resource")
> -	sz=$(echo $decoder | jq -r ".size")
> +	res=$(decimal_to_hex $(echo $decoder | jq -r ".resource"))
> +	sz=$(decimal_to_hex $(echo $decoder | jq -r ".size"))

This feels like overkill, I think bash arithmentic operations can handle
hex conversion and decimal comparison...

>  	[ $sz -ne $region_size ] && err \
>  	"$LINENO: decoder: $i sz: $sz region_size: $region_size"
>  	[ $res -ne $region_base ] && err \

...i.e. does this solve the issue?

@@ -138,9 +138,9 @@ do
 
        res=$(echo $decoder | jq -r ".resource")
        sz=$(echo $decoder | jq -r ".size")
-       [ $sz -ne $region_size ] && err \
+       (( sz != $region_size )) && err \
        "$LINENO: decoder: $i sz: $sz region_size: $region_size"
-       [ $res -ne $region_base ] && err \
+       (( $res != $region_base )) && err \
        "$LINENO: decoder: $i base: $res region_base: $region_base"
 done

RE: [ndctl PATCH 2/3] test/cxl-region-sysfs.sh: use operator '!=' to compare hexadecimal value

[Dan Williams]

Li Zhijian wrote:
> Fix errors:
> line 111: [: 0x80000000: integer expression expected
> line 112: [: 0x3ff110000000: integer expression expected
> line 141: [: 0x80000000: integer expression expected
> line 143: [: 0x3ff110000000: integer expression expected

Similar commentary on how found and how someone knows that they need
this patch, and maybe a note about which version of bash is upset about
this given this problem has been present for a long time without issue.

> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  test/cxl-region-sysfs.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
> index ded7aa1..89f21a3 100644
> --- a/test/cxl-region-sysfs.sh
> +++ b/test/cxl-region-sysfs.sh
> @@ -108,8 +108,8 @@ do
>  
>  	sz=$(cat /sys/bus/cxl/devices/$i/size)
>  	res=$(cat /sys/bus/cxl/devices/$i/start)
> -	[ $sz -ne $region_size ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
> -	[ $res -ne $region_base ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"
> +	[ "$sz" != "$region_size" ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
> +	[ "$res" != "$region_base" ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"

maybe use ((sz != region_size)) to make it explicit that this is an
arithmetic comparison? I would defer to Vishal's preference here.

RE: [ndctl PATCH 3/3] test/cxl-region-sysfs.sh: Fix cxl-region-sysfs.sh: line 107: [: missing `]'

[Dan Williams]

Li Zhijian wrote:
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Please no patches with empty changelogs. Commentary on the impact of the
change is always welcome.

Otherwise change looks good to me, and I wonder why this error is only
triggering now?

Acked-by: Dan Williams <dan.j.williams@intel.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 89f21a3..3878351 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
> 
> 

Re: [ndctl PATCH 2/3] test/cxl-region-sysfs.sh: use operator '!=' to compare hexadecimal value

[Zhijian Li (Fujitsu)]

On 07/12/2023 05:35, Dan Williams wrote:
> Li Zhijian wrote:
>> Fix errors:
>> line 111: [: 0x80000000: integer expression expected
>> line 112: [: 0x3ff110000000: integer expression expected
>> line 141: [: 0x80000000: integer expression expected
>> line 143: [: 0x3ff110000000: integer expression expected
> 
> Similar commentary on how found and how someone knows that they need
> this patch, and maybe a note about which version of bash is upset about
> this given this problem has been present for a long time without issue.

I think it impacted all version of BASH (tested on bash 4.1, 4.2, 5.0, 5.1)


> 
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   test/cxl-region-sysfs.sh | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
>> index ded7aa1..89f21a3 100644
>> --- a/test/cxl-region-sysfs.sh
>> +++ b/test/cxl-region-sysfs.sh
>> @@ -108,8 +108,8 @@ do
>>   
>>   	sz=$(cat /sys/bus/cxl/devices/$i/size)
>>   	res=$(cat /sys/bus/cxl/devices/$i/start)
>> -	[ $sz -ne $region_size ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
>> -	[ $res -ne $region_base ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"
>> +	[ "$sz" != "$region_size" ] && err "$LINENO: decoder: $i sz: $sz region_size: $region_size"
>> +	[ "$res" != "$region_base" ] && err "$LINENO: decoder: $i base: $res region_base: $region_base"
> 
> maybe use ((sz != region_size)) to make it explicit that this is an
> arithmetic comparison? I would defer to Vishal's preference here.

Per bash man page, we can also use [[ ]] instead of [ ], so that we are able to get rid of patch1

        arg1 OP arg2
               OP is one of -eq, -ne, -lt, -le, -gt, or -ge.  These arithmetic binary operators return true if arg1 is equal to, not equal to, less than, less than or equal to, greater than, or greater than or equal  to  arg2,
               respectively.  Arg1 and arg2 may be positive or negative integers.  When used with the [[ command, Arg1 and Arg2 are evaluated as arithmetic expressions  (see ARITHMETIC EVALUATION above).

Re: [ndctl PATCH 3/3] test/cxl-region-sysfs.sh: Fix cxl-region-sysfs.sh: line 107: [: missing `]'

[Zhijian Li (Fujitsu)]

On 07/12/2023 05:37, Dan Williams wrote:
> Li Zhijian wrote:
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> 
> Please no patches with empty changelogs. Commentary on the impact of the
> change is always welcome.
> 
> Otherwise change looks good to me, and I wonder why this error is only
> triggering now?


I have to say current condition checking 1) easily hides *BUG*
1) [ a -ne b ] && echo NG

Instead, 2) as below are more reliable.
2) [ a -eq b ] || echo NG



> 
> Acked-by: Dan Williams <dan.j.williams@intel.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 89f21a3..3878351 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
>>
>>
> 
>