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.or... [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,
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
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-- 2.43.0
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
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()
"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
! $ns_exist && ns_list="$ns_list $ns"
! $ns_exist && ns_list+=("$ns")
- 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)" 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}" ] && continueI 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.or... [1]
Cheers, Matt
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}" ] && continueI 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
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}" ] && continueI 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
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@@ -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
! $ns_exist && ns_list="$ns_list $ns"
! $ns_exist && ns_list+=("$ns")
- NS_LIST="$NS_LIST $ns_list"
- NS_LIST+=("${ns_list[@]}")
} tc_rule_stats_get()
-- 2.43.0
Reviewed-by: Hangbin Liu liuhangbin@gmail.com
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
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
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!
linux-kselftest-mirror@lists.linaro.org
-
Hangbin Liu -
Matthieu Baerts -
Matthieu Baerts (NGI0)
-
patchwork-bot+netdevbpf＠kernel.org -
Petr Machata