[PATCH net 0/3] selftests: net: lib: small fixes

[PATCH net 0/3] selftests: net: lib: small fixes

[Matthieu Baerts (NGI0)]

While looking at using 'lib.sh' for the MPTCP selftests [1], we found
some small issues with 'lib.sh'. Here they are:

- Patch 1: fix 'errexit' (set -e) support with busywait. 'errexit' is
  supported in some functions, not all. A fix for v6.8+.

- Patch 2: avoid confusing error messages linked to the cleaning part
  when the netns setup fails. A fix for v6.8+.

- Patch 3: set a variable as local to avoid accidentally changing the
  value of a another one with the same name on the caller side. A fix
  for v6.10-rc1+.

Link: https://lore.kernel.org/mptcp/5f4615c3-0621-43c5-ad25-55747a4350ce@kernel.org/T/ [1]
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (3):
      selftests: net: lib: support errexit with busywait
      selftests: net: lib: avoid error removing empty netns name
      selftests: net: lib: set 'i' as local

 tools/testing/selftests/net/lib.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
---
base-commit: a535d59432370343058755100ee75ab03c0e3f91
change-id: 20240605-upstream-net-20240605-selftests-net-lib-fixes-7a90a1a8d9d2

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH net 1/3] selftests: net: lib: support errexit with busywait

[Matthieu Baerts (NGI0)]

If errexit is enabled ('set -e'), loopy_wait -- or busywait and others
using it -- will stop after the first failure.

Note that if the returned status of loopy_wait is checked, and even if
errexit is enabled, Bash will not stop at the first error.

Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
Cc: stable@vger.kernel.org
Acked-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index edc030e81a46..a422e10d3d3a 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -67,9 +67,7 @@ loopy_wait()
 	while true
 	do
 		local out
-		out=$("$@")
-		local ret=$?
-		if ((!ret)); then
+		if out=$("$@"); then
 			echo -n "$out"
 			return 0
 		fi

-- 
2.43.0

Re: [PATCH net 1/3] selftests: net: lib: support errexit with busywait

[Hangbin Liu]

On Wed, Jun 05, 2024 at 11:21:16AM +0200, Matthieu Baerts (NGI0) wrote:
> If errexit is enabled ('set -e'), loopy_wait -- or busywait and others
> using it -- will stop after the first failure.
> 
> Note that if the returned status of loopy_wait is checked, and even if
> errexit is enabled, Bash will not stop at the first error.
> 
> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")

Not sure if the fixes tag should be c5341bcc337c ("selftests: mlxsw: Add a
self-test for port-default priority"), so the fixes could be backported to
stable kernel for forwarding/lib.sh. Others looks good to me.

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

> Cc: stable@vger.kernel.org
> Acked-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index edc030e81a46..a422e10d3d3a 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -67,9 +67,7 @@ loopy_wait()
>  	while true
>  	do
>  		local out
> -		out=$("$@")
> -		local ret=$?
> -		if ((!ret)); then
> +		if out=$("$@"); then
>  			echo -n "$out"
>  			return 0
>  		fi
> 
> -- 
> 2.43.0
> 

Re: [PATCH net 1/3] selftests: net: lib: support errexit with busywait

[Matthieu Baerts]

Hi Hangbin,

Thank you for your reply!

On 05/06/2024 11:44, Hangbin Liu wrote:
> On Wed, Jun 05, 2024 at 11:21:16AM +0200, Matthieu Baerts (NGI0) wrote:
>> If errexit is enabled ('set -e'), loopy_wait -- or busywait and others
>> using it -- will stop after the first failure.
>>
>> Note that if the returned status of loopy_wait is checked, and even if
>> errexit is enabled, Bash will not stop at the first error.
>>
>> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
> 
> Not sure if the fixes tag should be c5341bcc337c ("selftests: mlxsw: Add a
> self-test for port-default priority"), so the fixes could be backported to
> stable kernel for forwarding/lib.sh.

Good point, I didn't notice it was coming from there.

I don't think that's needed: 'net/forwarding/lib.sh' is far from being
compatible with 'errexit', please look at all the utilisations of '$?'.

For 'net/lib.sh', that's different: some functions explicitly check for
'errexit' support. If I'm not mistaken, I fixed the only place where it
was not compatible with 'errexit'.

> Others looks good to me.
> 
> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Thank you for the review!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Matthieu Baerts (NGI0)]

If there is an error to create the first netns with 'setup_ns()',
'cleanup_ns()' will be called with an empty string as first parameter.

The consequences is that 'cleanup_ns()' will try to delete an invalid
netns, and wait 20 seconds if the netns list is empty.

Instead of just checking if the name is not empty, convert the string
separated by spaces to an array. Manipulating the array is cleaner, and
calling 'cleanup_ns()' with an empty array will be a no-op.

Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
Cc: stable@vger.kernel.org
Acked-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index a422e10d3d3a..e2f51102d7e1 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -15,7 +15,7 @@ ksft_xfail=2
 ksft_skip=4
 
 # namespace list created by setup_ns
-NS_LIST=""
+NS_LIST=()
 
 ##############################################################################
 # Helpers
@@ -137,6 +137,7 @@ cleanup_ns()
 	fi
 
 	for ns in "$@"; do
+		[ -z "${ns}" ] && continue
 		ip netns delete "${ns}" &> /dev/null
 		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
 			echo "Warn: Failed to remove namespace $ns"
@@ -150,7 +151,7 @@ cleanup_ns()
 
 cleanup_all_ns()
 {
-	cleanup_ns $NS_LIST
+	cleanup_ns "${NS_LIST[@]}"
 }
 
 # setup netns with given names as prefix. e.g
@@ -159,7 +160,7 @@ setup_ns()
 {
 	local ns=""
 	local ns_name=""
-	local ns_list=""
+	local ns_list=()
 	local ns_exist=
 	for ns_name in "$@"; do
 		# Some test may setup/remove same netns multi times
@@ -175,13 +176,13 @@ setup_ns()
 
 		if ! ip netns add "$ns"; then
 			echo "Failed to create namespace $ns_name"
-			cleanup_ns "$ns_list"
+			cleanup_ns "${ns_list[@]}"
 			return $ksft_skip
 		fi
 		ip -n "$ns" link set lo up
-		! $ns_exist && ns_list="$ns_list $ns"
+		! $ns_exist && ns_list+=("$ns")
 	done
-	NS_LIST="$NS_LIST $ns_list"
+	NS_LIST+=("${ns_list[@]}")
 }
 
 tc_rule_stats_get()

-- 
2.43.0

Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Petr Machata]

"Matthieu Baerts (NGI0)" <matttbe@kernel.org> writes:

> If there is an error to create the first netns with 'setup_ns()',
> 'cleanup_ns()' will be called with an empty string as first parameter.
>
> The consequences is that 'cleanup_ns()' will try to delete an invalid
> netns, and wait 20 seconds if the netns list is empty.
>
> Instead of just checking if the name is not empty, convert the string
> separated by spaces to an array. Manipulating the array is cleaner, and
> calling 'cleanup_ns()' with an empty array will be a no-op.
>
> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
> Cc: stable@vger.kernel.org
> Acked-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index a422e10d3d3a..e2f51102d7e1 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -15,7 +15,7 @@ ksft_xfail=2
>  ksft_skip=4
>  
>  # namespace list created by setup_ns
> -NS_LIST=""
> +NS_LIST=()
>  
>  ##############################################################################
>  # Helpers
> @@ -137,6 +137,7 @@ cleanup_ns()
>  	fi
>  
>  	for ns in "$@"; do
> +		[ -z "${ns}" ] && continue

I think this is now irrelevant though? Now cleanup_ns() will be called
with no arguments for an empty NS list, so the loop does not even kick in.

>  		ip netns delete "${ns}" &> /dev/null
>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
>  			echo "Warn: Failed to remove namespace $ns"
> @@ -150,7 +151,7 @@ cleanup_ns()
>  
>  cleanup_all_ns()
>  {
> -	cleanup_ns $NS_LIST
> +	cleanup_ns "${NS_LIST[@]}"
>  }
>  
>  # setup netns with given names as prefix. e.g
> @@ -159,7 +160,7 @@ setup_ns()
>  {
>  	local ns=""
>  	local ns_name=""
> -	local ns_list=""
> +	local ns_list=()
>  	local ns_exist=
>  	for ns_name in "$@"; do
>  		# Some test may setup/remove same netns multi times
> @@ -175,13 +176,13 @@ setup_ns()
>  
>  		if ! ip netns add "$ns"; then
>  			echo "Failed to create namespace $ns_name"
> -			cleanup_ns "$ns_list"
> +			cleanup_ns "${ns_list[@]}"
>  			return $ksft_skip
>  		fi
>  		ip -n "$ns" link set lo up
> -		! $ns_exist && ns_list="$ns_list $ns"
> +		! $ns_exist && ns_list+=("$ns")
>  	done
> -	NS_LIST="$NS_LIST $ns_list"
> +	NS_LIST+=("${ns_list[@]}")
>  }
>  
>  tc_rule_stats_get()

Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Matthieu Baerts]

Hi Petr,

Thank you for the review!

On 05/06/2024 12:38, Petr Machata wrote:
> 
> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> writes:
> 
>> If there is an error to create the first netns with 'setup_ns()',
>> 'cleanup_ns()' will be called with an empty string as first parameter.
>>
>> The consequences is that 'cleanup_ns()' will try to delete an invalid
>> netns, and wait 20 seconds if the netns list is empty.
>>
>> Instead of just checking if the name is not empty, convert the string
>> separated by spaces to an array. Manipulating the array is cleaner, and
>> calling 'cleanup_ns()' with an empty array will be a no-op.
>>
>> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
>> Cc: stable@vger.kernel.org
>> Acked-by: Geliang Tang <geliang@kernel.org>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>> index a422e10d3d3a..e2f51102d7e1 100644
>> --- a/tools/testing/selftests/net/lib.sh
>> +++ b/tools/testing/selftests/net/lib.sh
>> @@ -15,7 +15,7 @@ ksft_xfail=2
>>  ksft_skip=4
>>  
>>  # namespace list created by setup_ns
>> -NS_LIST=""
>> +NS_LIST=()
>>  
>>  ##############################################################################
>>  # Helpers
>> @@ -137,6 +137,7 @@ cleanup_ns()
>>  	fi
>>  
>>  	for ns in "$@"; do
>> +		[ -z "${ns}" ] && continue
> 
> I think this is now irrelevant though? Now cleanup_ns() will be called
> with no arguments for an empty NS list, so the loop does not even kick in.

If you don't mind, I think it is "safer" to keep it: some selftests are
using 'cleanup_ns()' directly, not via 'cleanup_all_ns()', e.g.
netns-name.sh, cmsg-*.sh, fib-*.sh, etc. which can call it with the
variables not set if 'setup_ns' failed during the init phase.

For the moment, all these selftests are calling 'cleanup_ns()' with
parameters added without double quotes: so it is fine. Until someone
changes that to please shellcheck, like we did on our side with MPTCP
selftests. So this line will be useful soon when we will publish the
rest of our patches to use 'lib.sh' [1] :)

Link:
https://lore.kernel.org/mptcp/5f4615c3-0621-43c5-ad25-55747a4350ce@kernel.org/T/
[1]

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Petr Machata]

Matthieu Baerts <matttbe@kernel.org> writes:

> Hi Petr,
>
> Thank you for the review!
>
> On 05/06/2024 12:38, Petr Machata wrote:
>> 
>> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> writes:
>> 
>>> If there is an error to create the first netns with 'setup_ns()',
>>> 'cleanup_ns()' will be called with an empty string as first parameter.
>>>
>>> The consequences is that 'cleanup_ns()' will try to delete an invalid
>>> netns, and wait 20 seconds if the netns list is empty.
>>>
>>> Instead of just checking if the name is not empty, convert the string
>>> separated by spaces to an array. Manipulating the array is cleaner, and
>>> calling 'cleanup_ns()' with an empty array will be a no-op.
>>>
>>> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
>>> Cc: stable@vger.kernel.org
>>> Acked-by: Geliang Tang <geliang@kernel.org>
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>> ---
>>>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>>> index a422e10d3d3a..e2f51102d7e1 100644
>>> --- a/tools/testing/selftests/net/lib.sh
>>> +++ b/tools/testing/selftests/net/lib.sh
>>> @@ -15,7 +15,7 @@ ksft_xfail=2
>>>  ksft_skip=4
>>>  
>>>  # namespace list created by setup_ns
>>> -NS_LIST=""
>>> +NS_LIST=()
>>>  
>>>  ##############################################################################
>>>  # Helpers
>>> @@ -137,6 +137,7 @@ cleanup_ns()
>>>  	fi
>>>  
>>>  	for ns in "$@"; do
>>> +		[ -z "${ns}" ] && continue
>> 
>> I think this is now irrelevant though? Now cleanup_ns() will be called
>> with no arguments for an empty NS list, so the loop does not even kick in.
>
> If you don't mind, I think it is "safer" to keep it: some selftests are
> using 'cleanup_ns()' directly, not via 'cleanup_all_ns()', e.g.
> netns-name.sh, cmsg-*.sh, fib-*.sh, etc. which can call it with the
> variables not set if 'setup_ns' failed during the init phase.
>
> For the moment, all these selftests are calling 'cleanup_ns()' with
> parameters added without double quotes: so it is fine. Until someone
> changes that to please shellcheck, like we did on our side with MPTCP
> selftests. So this line will be useful soon when we will publish the
> rest of our patches to use 'lib.sh' [1] :)

All right.

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Matthieu Baerts]

On 05/06/2024 16:30, Petr Machata wrote:
> 
> Matthieu Baerts <matttbe@kernel.org> writes:
> 
>> Hi Petr,
>>
>> Thank you for the review!
>>
>> On 05/06/2024 12:38, Petr Machata wrote:
>>>
>>> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> writes:
>>>
>>>> If there is an error to create the first netns with 'setup_ns()',
>>>> 'cleanup_ns()' will be called with an empty string as first parameter.
>>>>
>>>> The consequences is that 'cleanup_ns()' will try to delete an invalid
>>>> netns, and wait 20 seconds if the netns list is empty.
>>>>
>>>> Instead of just checking if the name is not empty, convert the string
>>>> separated by spaces to an array. Manipulating the array is cleaner, and
>>>> calling 'cleanup_ns()' with an empty array will be a no-op.
>>>>
>>>> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
>>>> Cc: stable@vger.kernel.org
>>>> Acked-by: Geliang Tang <geliang@kernel.org>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>>>> index a422e10d3d3a..e2f51102d7e1 100644
>>>> --- a/tools/testing/selftests/net/lib.sh
>>>> +++ b/tools/testing/selftests/net/lib.sh
>>>> @@ -15,7 +15,7 @@ ksft_xfail=2
>>>>  ksft_skip=4
>>>>  
>>>>  # namespace list created by setup_ns
>>>> -NS_LIST=""
>>>> +NS_LIST=()
>>>>  
>>>>  ##############################################################################
>>>>  # Helpers
>>>> @@ -137,6 +137,7 @@ cleanup_ns()
>>>>  	fi
>>>>  
>>>>  	for ns in "$@"; do
>>>> +		[ -z "${ns}" ] && continue
>>>
>>> I think this is now irrelevant though? Now cleanup_ns() will be called
>>> with no arguments for an empty NS list, so the loop does not even kick in.
>>
>> If you don't mind, I think it is "safer" to keep it: some selftests are
>> using 'cleanup_ns()' directly, not via 'cleanup_all_ns()', e.g.
>> netns-name.sh, cmsg-*.sh, fib-*.sh, etc. which can call it with the
>> variables not set if 'setup_ns' failed during the init phase.
>>
>> For the moment, all these selftests are calling 'cleanup_ns()' with
>> parameters added without double quotes: so it is fine. Until someone
>> changes that to please shellcheck, like we did on our side with MPTCP
>> selftests. So this line will be useful soon when we will publish the
>> rest of our patches to use 'lib.sh' [1] :)
> 
> All right.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>

Thank you!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH net 2/3] selftests: net: lib: avoid error removing empty netns name

[Hangbin Liu]

On Wed, Jun 05, 2024 at 11:21:17AM +0200, Matthieu Baerts (NGI0) wrote:
> If there is an error to create the first netns with 'setup_ns()',
> 'cleanup_ns()' will be called with an empty string as first parameter.
> 
> The consequences is that 'cleanup_ns()' will try to delete an invalid
> netns, and wait 20 seconds if the netns list is empty.
> 
> Instead of just checking if the name is not empty, convert the string
> separated by spaces to an array. Manipulating the array is cleaner, and
> calling 'cleanup_ns()' with an empty array will be a no-op.
> 
> Fixes: 25ae948b4478 ("selftests/net: add lib.sh")
> Cc: stable@vger.kernel.org
> Acked-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index a422e10d3d3a..e2f51102d7e1 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -15,7 +15,7 @@ ksft_xfail=2
>  ksft_skip=4
>  
>  # namespace list created by setup_ns
> -NS_LIST=""
> +NS_LIST=()
>  
>  ##############################################################################
>  # Helpers
> @@ -137,6 +137,7 @@ cleanup_ns()
>  	fi
>  
>  	for ns in "$@"; do
> +		[ -z "${ns}" ] && continue
>  		ip netns delete "${ns}" &> /dev/null
>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
>  			echo "Warn: Failed to remove namespace $ns"
> @@ -150,7 +151,7 @@ cleanup_ns()
>  
>  cleanup_all_ns()
>  {
> -	cleanup_ns $NS_LIST
> +	cleanup_ns "${NS_LIST[@]}"
>  }
>  
>  # setup netns with given names as prefix. e.g
> @@ -159,7 +160,7 @@ setup_ns()
>  {
>  	local ns=""
>  	local ns_name=""
> -	local ns_list=""
> +	local ns_list=()
>  	local ns_exist=
>  	for ns_name in "$@"; do
>  		# Some test may setup/remove same netns multi times
> @@ -175,13 +176,13 @@ setup_ns()
>  
>  		if ! ip netns add "$ns"; then
>  			echo "Failed to create namespace $ns_name"
> -			cleanup_ns "$ns_list"
> +			cleanup_ns "${ns_list[@]}"
>  			return $ksft_skip
>  		fi
>  		ip -n "$ns" link set lo up
> -		! $ns_exist && ns_list="$ns_list $ns"
> +		! $ns_exist && ns_list+=("$ns")
>  	done
> -	NS_LIST="$NS_LIST $ns_list"
> +	NS_LIST+=("${ns_list[@]}")
>  }
>  
>  tc_rule_stats_get()
> 
> -- 
> 2.43.0
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

[PATCH net 3/3] selftests: net: lib: set 'i' as local

[Matthieu Baerts (NGI0)]

Without this, the 'i' variable declared before could be overridden by
accident, e.g.

  for i in "${@}"; do
      __ksft_status_merge "${i}"  ## 'i' has been modified
      foo "${i}"                  ## using 'i' with an unexpected value
  done

After a quick look, it looks like 'i' is currently not used after having
been modified in __ksft_status_merge(), but still, better be safe than
sorry. I saw this while modifying the same file, not because I suspected
an issue somewhere.

Fixes: 596c8819cb78 ("selftests: forwarding: Have RET track kselftest framework constants")
Acked-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index e2f51102d7e1..9155c914c064 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -27,6 +27,7 @@ __ksft_status_merge()
 	local -A weights
 	local weight=0
 
+	local i
 	for i in "$@"; do
 		weights[$i]=$((weight++))
 	done

-- 
2.43.0

Re: [PATCH net 3/3] selftests: net: lib: set 'i' as local

[Hangbin Liu]

On Wed, Jun 05, 2024 at 11:21:18AM +0200, Matthieu Baerts (NGI0) wrote:
> Without this, the 'i' variable declared before could be overridden by
> accident, e.g.
> 
>   for i in "${@}"; do
>       __ksft_status_merge "${i}"  ## 'i' has been modified
>       foo "${i}"                  ## using 'i' with an unexpected value
>   done
> 
> After a quick look, it looks like 'i' is currently not used after having
> been modified in __ksft_status_merge(), but still, better be safe than
> sorry. I saw this while modifying the same file, not because I suspected
> an issue somewhere.
> 
> Fixes: 596c8819cb78 ("selftests: forwarding: Have RET track kselftest framework constants")
> Acked-by: Geliang Tang <geliang@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
> index e2f51102d7e1..9155c914c064 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -27,6 +27,7 @@ __ksft_status_merge()
>  	local -A weights
>  	local weight=0
>  
> +	local i
>  	for i in "$@"; do
>  		weights[$i]=$((weight++))
>  	done
> 
> -- 
> 2.43.0
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH net 0/3] selftests: net: lib: small fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 05 Jun 2024 11:21:15 +0200 you wrote:
> While looking at using 'lib.sh' for the MPTCP selftests [1], we found
> some small issues with 'lib.sh'. Here they are:
> 
> - Patch 1: fix 'errexit' (set -e) support with busywait. 'errexit' is
>   supported in some functions, not all. A fix for v6.8+.
> 
> - Patch 2: avoid confusing error messages linked to the cleaning part
>   when the netns setup fails. A fix for v6.8+.
> 
> [...]

Here is the summary with links:
  - [net,1/3] selftests: net: lib: support errexit with busywait
    https://git.kernel.org/netdev/net/c/41b02ea4c0ad
  - [net,2/3] selftests: net: lib: avoid error removing empty netns name
    https://git.kernel.org/netdev/net/c/79322174bcc7
  - [net,3/3] selftests: net: lib: set 'i' as local
    https://git.kernel.org/netdev/net/c/84a8bc3ec225

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html