Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0) wrote:
> Instead of only appending items to the list, remove them when the
> netns
> has been deleted.
> 
> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
> remove already deleted netns.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib.sh
> b/tools/testing/selftests/net/lib.sh
> index b2572aff6286..c7a8cfb477cc 100644
> --- a/tools/testing/selftests/net/lib.sh
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -125,6 +125,20 @@ slowwait_for_counter()
>  	slowwait "$timeout" until_counter_is ">= $((base + delta))"
> "$@"
>  }
>  
> +remove_ns_list()
> +{
> +	local item=$1
> +	local ns
> +	local ns_list=("${NS_LIST[@]}")
> +	NS_LIST=()
> +
> +	for ns in "${ns_list[@]}"; do
> +		if [ "${ns}" != "${item}" ]; then
> +			NS_LIST+=("${ns}")
> +		fi
> +	done
> +}
> +
>  cleanup_ns()
>  {
>  	local ns=""
> @@ -136,6 +150,8 @@ cleanup_ns()
>  		if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
> grep -vq "^$ns$" &> /dev/null; then
>  			echo "Warn: Failed to remove namespace $ns"
>  			ret=1
> +		else

nit: Should we also remove ns_list when "ip netns list" is busy? Or
should we not delete ns when "ip netns list" is busy?

I think we should do these two commands together:

	ip netns delete "${ns}" &> /dev/null || true
	remove_ns_list "${ns}"

I'm not sure. WDYT?

Thanks,
-Geliang

> +			remove_ns_list "${ns}"
>  		fi
>  	done
>  
> @@ -154,17 +170,14 @@ setup_ns()
>  	local ns=""
>  	local ns_name=""
>  	local ns_list=()
> -	local ns_exist=
>  	for ns_name in "$@"; do
>  		# 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_exist=false
>  		else
>  			eval ns='$'${ns_name}
>  			cleanup_ns "$ns"
> -			ns_exist=true
>  		fi
>  
>  		if ! ip netns add "$ns"; then
> @@ -173,7 +186,7 @@ setup_ns()
>  			return $ksft_skip
>  		fi
>  		ip -n "$ns" link set lo up
> -		! $ns_exist && ns_list+=("$ns")
> +		ns_list+=("$ns")
>  	done
>  	NS_LIST+=("${ns_list[@]}")
>  }
>