Re: [PATCH mptcp-next v4 1/6] selftests: net: rename ns in setup/cleanup_ns

[Matthieu Baerts <matttbe@kernel.org>]

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.