Re: [PATCH mptcp-next 2/5] selftests: mptcp: add mptcp_lib_ns_* helpers

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 19/02/2024 10:29, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add helpers mptcp_lib_ns_init() and mptcp_lib_ns_exit() in mptcp_lib.sh to
> init all namespaces ns1, ns2, ns3 and ns4. Then every test script can
> invoke these helpers and use all namespaces.

Good idea to move duplicated code to the _lib.sh file.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 6d9a2af85a8d..bfc535594675 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -350,3 +350,62 @@ mptcp_lib_check_tools() {
>  		esac
>  	done
>  }
> +
> +rndh=""

I guess you "export" it because it is needed for the captured files,
right? If yes, why not using the "${ns}" variables in a dedicated commit
before to ease this refactoring?

> +ns1=""
> +ns2=""
> +ns3=""
> +ns4=""

I really don't think it is a good idea to hide these variables: if you
open 'mptcp_connect.sh', it will be confusing not to see where these
variables have been declared. If you declare variables (or functions) in
the lib, they should be prefixed MPTCP_LIB_ (or mptcp_lib_ for the
functions) I think.

Instead, I think you should keep the ns* variables declared at the top
of each file, but call "mptcp_lib_ns_init" with the variables to
initiate, e.g.

  ns1=""
  ns2=""
  ns_disabled=""

  (...)

  mptcp_lib_ns_init ns1 ns2 ns_disabled

in mptcp_lib_ns_init(), you can do something like:

  (...)

  local ns_i
  for ns_i in "${@}"; do
      eval "${ns_i}=${ns_i}-${rndh}"

      ip netns add "${!ns_i}" || (...)

(so you can use the version with '!' in the for loop)

> +mptcp_lib_ns_init() {
> +	: "${rndh?}"
> +	: "${ns1?}"
> +	: "${ns2?}"
> +	: "${ns3?}"
> +	: "${ns4?}"
> +
> +	local nr="${1:-4}"
> +	local i=1
> +	local sec
> +
> +	sec=$(date +%s)
> +	rndh=$(printf %x "$sec")-$(mktemp -u XXXXXX)
> +
> +	ns1="ns1-$rndh"
> +	ns2="ns2-$rndh"
> +	ns3="ns3-$rndh"
> +	ns4="ns4-$rndh"

In this mptcp_lib.sh, probably good to continue to use {} around
variables ('${rndh}' instead of '$rndh') like in the rest of the file:
clearer, and we avoid typos.

> +
> +	local netns
> +	for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
> +		if [ "$i" -gt "$nr" ]; then
> +			break
> +		fi

(see above)

> +		ip netns add "$netns" || exit ${KSFT_SKIP}
> +		ip -net "$netns" link set lo up
> +
> +		ip netns exec "$netns" sysctl -q net.ipv4.conf.all.rp_filter=0
> +		ip netns exec "$netns" sysctl -q net.ipv4.conf.default.rp_filter=0

Mmh, is it OK to do that for all netns? We were not doing that before
for diag.sh, but I suppose that's because it was not communicating with
other netns, right? So fine to do that everywhere?

That's a behaviour change. Please always clearly mention behaviour
changes in the commit message when you do such refactoring, where we
would expect no behaviour changes.

While at it, maybe we want to add:

  ip netns exec "${!ns_i}" sysctl -q net.mptcp.enabled=1

That should be the default value. If not, good to override. (I think it
is needed for some RHEL versions.) If yes, please add that in the commit
message as well.

> +		i=$((i + 1))
> +	done
> +}
> +
> +mptcp_lib_ns_exit() {
> +	: "${ns1:?}"
> +	: "${ns2:?}"
> +	: "${ns3:?}"
> +	: "${ns4:?}"
> +
> +	local nr="${1:-4}"
> +	local i=1
> +
> +	local netns
> +	for netns in "$ns1" "$ns2" "$ns3" "$ns4"; do
> +		if [ "$i" -gt "$nr" ]; then
> +			break
> +		fi

Same here, best to clearer to pass the variables names and accept a
variable number of parameters:


  mptcp_lib_ns_exit "${ns1}" "${ns2}"

  local ns_i
  for ns_i in "${@}"; do
      ip netns del "${ns_i}"

> +		ip netns del "$netns"
> +		rm -f /tmp/"$netns".{nstat,out}

Here, it is also a behaviour change. I guess that's fine, but still,
please mention that in the commit message.

> +		i=$((i + 1))
> +	done
> +}

(...)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.