Re: [PATCH mptcp-next v3 3/3] selftests: mptcp: add mptcp_lib_evts_* helpers

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 21/02/2024 8:06 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To avoid duplicated code in different MPTCP selftests, we can add and
> use helpers defined in mptcp_lib.sh.
> 
> This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
> and mptcp_join.sh into four helpers: mptcp_lib_evts_init(), _start(),
> _kill() and _remove(). Define them in mptcp_lib.sh and use these new
> helpers in both scripts.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 16 ++----
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 55 +++++++++++++++++++
>  .../selftests/net/mptcp/userspace_pm.sh       | 28 +++-------
>  3 files changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index bea7627c7313..006fefaf2e2a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -158,8 +158,7 @@ init() {
>  	cinsent=$(mktemp)
>  	cout=$(mktemp)
>  	err=$(mktemp)
> -	evts_ns1=$(mktemp)
> -	evts_ns2=$(mktemp)
> +	mptcp_lib_evts_init evts_ns1 evts_ns2

Maybe easier to keep this unchanged? All you do in the lib is:

  <var>=$(mktemp)

Same for the mptcp_lib_evts_remove -- just a 'rm' -- and
mptcp_lib_evts_kill -- just calling mptcp_lib_kill_wait.

Or do you see other advantages?

>  	trap cleanup EXIT
>  

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 14c0b97a153e..9573edb4efb1 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -396,3 +396,58 @@ mptcp_lib_ns_exit() {
>  		ip netns del "$netns"
>  	done
>  }
> +
> +mptcp_lib_evts_init() {
> +	local arg
> +
> +	for arg in "${@}"; do
> +		declare -n evts="${arg}"
> +
> +		if [ -z "${evts}" ]; then
> +			evts=$(mktemp)
> +		fi
> +	done
> +}

(see above, maybe we don't need this helper?)

> +
> +# $1 ns1, $2 ns2
> +mptcp_lib_evts_start() {
> +	local ns_1="${1}"
> +	local ns_2="${2}"
> +	local evts_1="${3}"
> +	local evts_2="${4}"
> +	declare -n pid_1="${5}"
> +	declare -n pid_2="${6}"

(or use ${!pid_1} as in mptcp_lib_ns_init -- or we use 'declare -n'
there too, just for consistency)

(I hope 'declare -n' will respect the 'local' scope, but I guess yes)

> +
> +	:>"${evts_1}"
> +	:>"${evts_2}"
> +
> +	if [ ${pid_1} -ne 0 ]; then
> +		mptcp_lib_kill_wait "${pid_1}"
> +	fi
> +	ip netns exec "${ns_1}" ./pm_nl_ctl events >> "${evts_1}" 2>&1 &
> +	pid_1=$!
> +
> +	if [ ${pid_2} -ne 0 ]; then
> +		mptcp_lib_kill_wait "${pid_2}"
> +	fi
> +	ip netns exec "${ns_2}" ./pm_nl_ctl events >> "${evts_2}" 2>&1 &
> +	pid_2=$!
> +}
> +
> +mptcp_lib_evts_kill() {
> +	declare -n pid_1="${1}"
> +	declare -n pid_2="${2}"
> +
> +	mptcp_lib_kill_wait "${pid_1}"
> +	mptcp_lib_kill_wait "${pid_2}"
> +
> +	pid_1=0
> +	pid_2=0

This was not done before, do you need that?

(but see above, maybe we don't need this helper, and the one below)

> +}
> +
> +mptcp_lib_evts_remove() {
> +	local evts_1="${1}"
> +	local evts_2="${2}"
> +
> +	rm -rf "${evts_1}" "${evts_2}"
> +}

(...)

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