From: Geliang Tang <tanggeliang@kylinos.cn>
The helpers setup_ns and cleanup_ns don't work when a namespace named
"ns" is passed to them.
For example, in net/mptcp/diag.sh, the name of the namespace is "ns".
If "setup_ns ns" is used in it, diag.sh fails with errors:
Invalid netns name "./mptcp_connect"
Cannot open network namespace "10000": No such file or directory
Cannot open network namespace "10000": No such file or directory
That is because "ns" is also a local variable in both setup_ns and
cleanup_ns. To solve this, this patch renames the local variable "ns"
as "_ns".
Also a ns_name valid check has been added to setup_ns. If "_ns" is
passed in as a ns_name, setup_ns helper exits.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/net/lib.sh | 33 ++++++++++++++++++------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index edc030e81a46..ce611bc73098 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -128,7 +128,7 @@ slowwait_for_counter()
cleanup_ns()
{
- local ns=""
+ local _ns=""
local errexit=0
local ret=0
@@ -138,10 +138,11 @@ cleanup_ns()
set +e
fi
- for ns in "$@"; do
- 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"
+ for _ns in "$@"; do
+ 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"
ret=1
fi
done
@@ -159,29 +160,35 @@ cleanup_all_ns()
# setup_ns local remote
setup_ns()
{
- local ns=""
+ local _ns=""
local ns_name=""
local ns_list=""
local ns_exist=
for ns_name in "$@"; do
+ if [ "${ns_name}" == "_ns" ]; then
+ echo "Failed to setup namespace '${ns_name}': invalid name"
+ cleanup_ns "$ns_list"
+ set -e
+ return $ksft_fail
+ fi
# Some test may setup/remove same netns multi times
if unset ${ns_name} 2> /dev/null; then
- ns="${ns_name,,}-$(mktemp -u XXXXXX)"
- eval readonly ${ns_name}="$ns"
+ _ns="${ns_name,,}-$(mktemp -u XXXXXX)"
+ eval readonly ${ns_name}="$_ns"
ns_exist=false
else
- eval ns='$'${ns_name}
- cleanup_ns "$ns"
+ eval _ns='$'${ns_name}
+ cleanup_ns "$_ns"
ns_exist=true
fi
- if ! ip netns add "$ns"; then
+ if ! ip netns add "$_ns"; then
echo "Failed to create namespace $ns_name"
cleanup_ns "$ns_list"
return $ksft_skip
fi
- ip -n "$ns" link set lo up
- ! $ns_exist && ns_list="$ns_list $ns"
+ ip -n "$_ns" link set lo up
+ ! $ns_exist && ns_list="$ns_list $_ns"
done
NS_LIST="$NS_LIST $ns_list"
}
--
2.43.0
Hi Geliang, On 24/05/2024 08:48, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The helpers setup_ns and cleanup_ns don't work when a namespace named > "ns" is passed to them. > > For example, in net/mptcp/diag.sh, the name of the namespace is "ns". > If "setup_ns ns" is used in it, diag.sh fails with errors: > > Invalid netns name "./mptcp_connect" > Cannot open network namespace "10000": No such file or directory > Cannot open network namespace "10000": No such file or directory > > That is because "ns" is also a local variable in both setup_ns and > cleanup_ns. To solve this, this patch renames the local variable "ns" > as "_ns". Do you mind justifying why the new name helps? Something like: (...) as "_ns", which is more unlikely to conflict with existing variables. "_ns" name appears to be unused in the selftests. > Also a ns_name valid check has been added to setup_ns. If "_ns" is > passed in as a ns_name, setup_ns helper exits. > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > tools/testing/selftests/net/lib.sh | 33 ++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh > index edc030e81a46..ce611bc73098 100644 > --- a/tools/testing/selftests/net/lib.sh > +++ b/tools/testing/selftests/net/lib.sh > @@ -128,7 +128,7 @@ slowwait_for_counter() > > cleanup_ns() > { > - local ns="" > + local _ns="" Are you sure this modification in cleanup_ns is needed as well? The variable is local, and it is not unset / modified using the name passed in argument. > local errexit=0 > local ret=0 > > @@ -138,10 +138,11 @@ cleanup_ns() > set +e > fi > > - for ns in "$@"; do > - 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" > + for _ns in "$@"; do > + 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" > ret=1 > fi > done > @@ -159,29 +160,35 @@ cleanup_all_ns() > # setup_ns local remote > setup_ns() > { > - local ns="" > + local _ns="" > local ns_name="" > local ns_list="" > local ns_exist= > for ns_name in "$@"; do > + if [ "${ns_name}" == "_ns" ]; then > + echo "Failed to setup namespace '${ns_name}': invalid name" > + cleanup_ns "$ns_list" If the list is empty, it will call cleanup_ns with "", and it will try to do 'ip netns delete ""', etc. If ns_list is an array, then 'cleanup_ns "${ns_list[@]}"' is OK. Mmh, I see there is the same issue below. I can send a fix for that, that would be for -net, not next. I will look at that this afternoon. > + set -e Why do you need this? (see below) > + return $ksft_fail Why not 'exit ${ksft_fail}' instead? That's an internal error, you can do that. > + fi > # Some test may setup/remove same netns multi times > if unset ${ns_name} 2> /dev/null; then > - ns="${ns_name,,}-$(mktemp -u XXXXXX)" > - eval readonly ${ns_name}="$ns" > + _ns="${ns_name,,}-$(mktemp -u XXXXXX)" > + eval readonly ${ns_name}="$_ns" > ns_exist=false > else > - eval ns='$'${ns_name} > - cleanup_ns "$ns" > + eval _ns='$'${ns_name} > + cleanup_ns "$_ns" > ns_exist=true > fi > > - if ! ip netns add "$ns"; then > + if ! ip netns add "$_ns"; then > echo "Failed to create namespace $ns_name" > cleanup_ns "$ns_list" > return $ksft_skip > fi > - ip -n "$ns" link set lo up > - ! $ns_exist && ns_list="$ns_list $ns" > + ip -n "$_ns" link set lo up > + ! $ns_exist && ns_list="$ns_list $_ns" > done > NS_LIST="$NS_LIST $ns_list" > } Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.