Re: [PATCH mptcp-next 07/10] selftests: mptcp: add endpoint_ops API helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 11/03/2024 02:48, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch moves six endpoint operation helpers with the pm_nl_ prefix:
> 
> 	pm_nl_limits(),
> 	pm_nl_add_endpoint(),
> 	pm_nl_del_endpoint(),
> 	pm_nl_flush_endpoint(),
> 	pm_nl_show_endpoints() and
> 	pm_nl_change_endpoint()
> 
> from mptcp_join.sh into mptcp_lib.sh as public functions, and renamed each
> of them with a mptcp_lib_ prefix.
> 
> Add a new helper mptcp_lib_endpoint_ops() in mptcp_lib.sh as the API for
> all endpoint operation helpers, which invokes each of mptcp_lib_ prefix
> helpers according to the first argument of it is "limits", "add", "del",
> "flush", "show" or "change".
> 
> Usage:
>         mptcp_lib_endpoint_ops limits $ns $addrs $subflows
>         mptcp_lib_endpoint_ops add $ns $addr
>         mptcp_lib_endpoint_ops del $ns $id $addr
>         mptcp_lib_endpoint_ops flush $ns
>         mptcp_lib_endpoint_ops show $ns
>         mptcp_lib_endpoint_ops change $ns $id $flags

I'm not sure to understand why you called this helper "endpoint". With
"pm_nl", it means it is linked to the Path Manager, the Netlink
interface. Maybe best to use 'mptcp_lib_pm_nl_ops'?

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index c465f1d59419..d395692fac0d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -523,3 +523,151 @@ mptcp_lib_is_ip_mptcp() {
>  
>  	[ ${mptcp_lib_ip_mptcp} -eq 1 ]
>  }
> +
> +mptcp_lib_limits() {

Same here for the prefix, "limits" is too vague:

  mptcp_lib_pm_nl_limits()

> +	local ns=${1}
> +	local addrs=${2}
> +	local subflows=${3}
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		local limits="limits"
> +
> +		if [ -n "${addrs}" ] && [ -n "${subflows}" ]; then
> +			limits+=" set add_addr_accepted ${addrs} subflows ${subflows}"
> +		fi
> +		# shellcheck disable=SC2086

Please *always* *always* justify why you ignore a rule, e.g.

  # shellcheck disable=SCXXXX  ## <reason>

or

  # <long description>
  # <on multiple lines>
  # shellcheck disable=SCXXXX


(I guess you don't need to ignore that if you look at my comment on
patch 2/10, same below)

> +		ip -n "${ns}" mptcp ${limits}
> +	else
> +		# shellcheck disable=SC2086
> +		ip netns exec "${ns}" ./pm_nl_ctl limits "${addrs}" ${subflows}
> +	fi
> +}
> +
> +mptcp_lib_add_endpoint() {
> +	local ns=${1}
> +	local addr=${2}
> +	local flags _flags
> +	local port _port
> +	local dev _dev
> +	local id _id
> +	local nr=2
> +
> +	local p
> +	for p in "${@}"
> +	do
> +		if [ "${p}" = "flags" ]; then
> +			eval _flags=\$"${nr}"
> +			[ -n "${_flags}" ]; flags="flags ${_flags}"
> +		fi
> +		if [ "${p}" = "dev" ]; then
> +			eval _dev=\$"${nr}"
> +			[ -n "${_dev}" ]; dev="dev ${_dev}"
> +		fi
> +		if [ "${p}" = "id" ]; then
> +			eval _id=\$"${nr}"
> +			[ -n "${_id}" ]; id="id ${_id}"
> +		fi
> +		if [ "${p}" = "port" ]; then
> +			eval _port=\$"${nr}"
> +			[ -n "${_port}" ]; port="port ${_port}"
> +		fi
> +
> +		nr=$((nr + 1))
> +	done
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		# shellcheck disable=SC2086

I wonder if you couldn't use arrays:

  dev=(dev "${_dev}")
  (...)
  ip (...) "${dev[@]}")

> +		ip -n "${ns}" mptcp endpoint add "${addr}" ${_flags//","/" "} ${dev} ${id} ${port}
> +	else
> +		# shellcheck disable=SC2086
> +		ip netns exec "${ns}" ./pm_nl_ctl add "${addr}" ${flags} ${dev} ${id} ${port}
> +	fi
> +}
> +
> +mptcp_lib_del_endpoint() {
> +	local ns=${1}
> +	local id=${2}
> +	local addr=${3}
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		[ "${id}" -ne 0 ] && addr=''
> +		# shellcheck disable=SC2086
> +		ip -n "${ns}" mptcp endpoint delete id "${id}" ${addr}

or something like that to avoid the disable:

  ${addr:+"${addr}"}

> +	else
> +		ip netns exec "${ns}" ./pm_nl_ctl del "${id}" "${addr}"
> +	fi
> +}
> +
> +mptcp_lib_flush_endpoint() {
> +	local ns=${1}
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		ip -n "${ns}" mptcp endpoint flush
> +	else
> +		ip netns exec "${ns}" ./pm_nl_ctl flush
> +	fi
> +}
> +
> +mptcp_lib_show_endpoints() {

(I guess this one is only needed in pm_netlink.sh, no?)

> +	local ns=${1}
> +	local id=${2}
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		local show="show"
> +
> +		[ -n "${id}" ] && show+=" id ${id}"
> +		# shellcheck disable=SC2086
> +		ip -n "${ns}" mptcp endpoint ${show}

Maybe this to avoid the disable:

  ip -n "${ns}" mptcp endpoint show ${id:+id "${id}"}

Same below

> +	else
> +		local dump="dump"
> +
> +		[ -n "${id}" ] && dump="get ${id}"
> +		# shellcheck disable=SC2086
> +		ip netns exec "${ns}" ./pm_nl_ctl ${dump}
> +	fi
> +}
> +
> +mptcp_lib_change_endpoint() {

(I guess this one is only needed in pm_netlink.sh, no?)

> +	local ns=${1}
> +	local addr=${2}
> +	local flags=${3}
> +
> +	if ! mptcp_lib_is_addr "${addr}"; then
> +		[ "${addr}" -gt 0 ] && [ "${addr}" -lt 256 ] && addr="id ${addr}"

use an array?

  XX=("${addr}")
  XX=("id ${addr}")
  ${XX[@]}

> +	fi
> +
> +	if mptcp_lib_is_ip_mptcp; then
> +		# shellcheck disable=SC2086
> +		ip -n "${ns}" mptcp endpoint change ${addr} ${flags//","/" "}
> +	else
> +		# shellcheck disable=SC2086
> +		ip netns exec "${ns}" ./pm_nl_ctl set ${addr} flags "${flags}"
> +	fi
> +}
> +
> +mptcp_lib_endpoint_ops() {
> +	[ "$#" -lt 2 ] && return 1
> +
> +	case "$1" in

(missing {} → ${1})

> +	"limits")
> +		mptcp_lib_limits "${@:2}"
> +		;;
> +	"add")
> +		mptcp_lib_add_endpoint "${@:2}"
> +		;;
> +	"del")
> +		mptcp_lib_del_endpoint "${@:2}"
> +		;;
> +	"flush")
> +		mptcp_lib_flush_endpoint "${@:2}"
> +		;;
> +	"show")
> +		mptcp_lib_show_endpoints "${@:2}"
> +		;;
> +	"change")
> +		mptcp_lib_change_endpoint "${@:2}"
> +		;;
> +	*)
> +		;;
> +	esac
> +}

I'm not sure to see the advantage of using mptcp_lib_endpoint_ops(), why
not directly calling mptcp_lib_pm_nl_limits, etc.?

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