| Message ID | 7a877165562901175b913285b3bb844f2cbc49c9.1716533107.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | use helpers in lib.sh and net_helpers.sh | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 66 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
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
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" }