Re: [PATCH mptcp-next v3 22/29] selftests: mptcp: add mptcp_lib_evts_*

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> This patch unifies "pm_nl_ctl events" related code in userspace_pm.sh
> and mptcp_join.sh into four functions: _init, _start, _kill and _remove.
> Define them in mptcp_lib.sh and use these new helper in both scripts.

Good idea to avoid duplicated code!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 8051ac5507dc..899c21ced5a3 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -220,6 +220,11 @@ mptcp_lib_evts_get_info() {
>  	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
>  }
>  
> +server_evts=""
> +client_evts=""
> +server_evts_pid=0
> +client_evts_pid=0

I wonder if it wouldn't be clearer to prefix these variables with
'mptcp_lib_': we would avoid overriding global variables by mistake from
other fiels and it would be clearer to know where these variables have
been defined. But I guess if we do that, we would need to do quite a few
modifications in userspace_pm.sh and we want to avoid that, no?

Or passing them as arguments to 'mptcp_lib_evts_XXX()'? It would be more
explicit but maybe not worth it?

An alternative could be to keep these variables declared in the
different files and keep these helpers as they are here. In this case,
it would be good to add a comment before mptcp_lib_evts_init()
explaining that these variables are needed.

Please also make sure shellcheck if happy with that, e.g. by adding a
'?' in the variable name like ${server_evts?} or explicitly at the
beginning of the function with something like ': "${var?}"' or ':
"${var:?}"' if it has to be non empty, e.g.

  mptcp_lib_evts_init() {
      : "${server_evts?}"
      : "${client_evts?}"

      (...)
  }

  mptcp_lib_evts_start() {
      : "${server_evts:?}"
      : "${client_evts:?}"
      : "${server_evts_pid:?}"
      : "${client_evts_pid:?}"

      (...)
  }

  mptcp_lib_evts_kill() {
      mptcp_lib_kill_wait "${server_evts_pid:?}"
      mptcp_lib_kill_wait "${client_evts_pid:?}"
  }

  mptcp_lib_evts_remove() {
      rm -rf "${server_evts:?}" "${client_evts:?}"
  }

> +
>  mptcp_lib_kill_wait() {
>  	[ $1 -eq 0 ] && return 0
>  
> @@ -227,3 +232,38 @@ mptcp_lib_kill_wait() {
>  	kill $1 > /dev/null 2>&1
>  	wait $1 2>/dev/null
>  }
> +
> +mptcp_lib_evts_init() {
> +	if [ -z "$server_evts" ]; then

Same here and below for the {}. (see comment from patch 21/29)

+ the double quotes below to keep shellcheck happy and be safer.

> +		server_evts=$(mktemp)
> +	fi
> +	if [ -z "$client_evts" ]; then
> +		client_evts=$(mktemp)
> +	fi
> +}
> +
> +mptcp_lib_evts_start() {
> +	:>"$server_evts"
> +	:>"$client_evts"
> +
> +	if [ $server_evts_pid -ne 0 ]; then
> +		mptcp_lib_kill_wait $server_evts_pid
> +	fi
> +	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &

I think ${ns1} and ${ns2} should be passed as argument to this function.

> +	server_evts_pid=$!
> +
> +	if [ $client_evts_pid -ne 0 ]; then
> +		mptcp_lib_kill_wait $client_evts_pid
> +	fi
> +	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
> +	client_evts_pid=$!
> +}
> +
> +mptcp_lib_evts_kill() {
> +	mptcp_lib_kill_wait $server_evts_pid
> +	mptcp_lib_kill_wait $client_evts_pid

Should we eventually set the pid to 0 after that just to be on the safe
side?

  server_evts_pid=0
  client_evts_pid=0

It was not needed before, maybe fine not to do that? Up to you (but if
you do, please add a note in the commit message)

> +}
> +
> +mptcp_lib_evts_remove() {
> +	rm -rf $server_evts $client_evts
> +}

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net