Re: [PATCH mptcp-next v3 21/29] selftests: mptcp: add mptcp_lib_kill_wait

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> Export kill_wait() helper in userspace_pm.sh into mptcp_lib.sh and
> rename it as mptcp_lib_kill_wait(). It can be used to instead of
> kill_wait() in mptcp_join.sh. Use the new helper in both scripts.

Good idea!

Just to make things 100% clear, please always start by explaining the
reason why this patch is interesting. Here it can simply be:

  To avoid duplicated code in different MPTCP selftests, we can add
  and use helpers defined in mptcp_lib.sh.

Same in the following commits. You can even copy-paste this explanation
in the next ones.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index bb95dd967eb3..8051ac5507dc 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -219,3 +219,11 @@ mptcp_lib_get_info_value() {
>  mptcp_lib_evts_get_info() {
>  	cat "${2}" | mptcp_lib_get_info_value "${1}" "^type:${3:-1},"
>  }
> +
> +mptcp_lib_kill_wait() {

Do you mind adding a comment just above mentioning the parameters that
are accepted? Like it is done in other functions, e.g.:

  # $1: PID

> +	[ $1 -eq 0 ] && return 0

Could you add "{}" (${1}) like it is done in the rest of the file, just
to keep the uniformity? (and because it *seems* better to have an
explicit delimiter in Bash to avoid mistakes)

Also, please also add double quotes around variables ("${1}") to keep
shellcheck happy (and because it is safer and for the uniformity).

> +
> +	kill -SIGUSR1 $1 > /dev/null 2>&1
> +	kill $1 > /dev/null 2>&1
> +	wait $1 2>/dev/null

(Same here and in the following commits)

Regarding ShellCheck, please always run it on the different .sh scripts
not to introduce new issues.

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