| Message ID | 20240605-upstream-net-20240605-selftests-net-lib-fixes-v1-2-b3afadd368c9@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: net: lib: small fixes | expand |
"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()
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
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>
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
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>
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()
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(-)