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.
