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/[email protected]/T/ [1]
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
---
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) <[email protected]>
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: [email protected]
Acked-by: Geliang Tang <[email protected]>
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
---
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
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: [email protected]
Acked-by: Geliang Tang <[email protected]>
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
---
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
"Matthieu Baerts (NGI0)" <[email protected]> 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: [email protected]
> Acked-by: Geliang Tang <[email protected]>
> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
> ---
> 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()
Hi Petr,
Thank you for the review!
On 05/06/2024 12:38, Petr Machata wrote:
>
> "Matthieu Baerts (NGI0)" <[email protected]> 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: [email protected]
>> Acked-by: Geliang Tang <[email protected]>
>> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
>> ---
>> 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/[email protected]/T/
[1]
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On 05/06/2024 16:30, Petr Machata wrote:
>
> Matthieu Baerts <[email protected]> writes:
>
>> Hi Petr,
>>
>> Thank you for the review!
>>
>> On 05/06/2024 12:38, Petr Machata wrote:
>>>
>>> "Matthieu Baerts (NGI0)" <[email protected]> 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: [email protected]
>>>> Acked-by: Geliang Tang <[email protected]>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
Thank you!
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Matthieu Baerts <[email protected]> writes:
> Hi Petr,
>
> Thank you for the review!
>
> On 05/06/2024 12:38, Petr Machata wrote:
>>
>> "Matthieu Baerts (NGI0)" <[email protected]> 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: [email protected]
>>> Acked-by: Geliang Tang <[email protected]>
>>> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
>>> ---
>>> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
> ---
> 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 <[email protected]>
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: [email protected]
> Acked-by: Geliang Tang <[email protected]>
> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
> ---
> 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 <[email protected]>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
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