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

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 107 +------------
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 148 ++++++++++++++++++
 2 files changed, 155 insertions(+), 100 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 431233e25abc..8e35f875147b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -603,132 +603,39 @@ kill_events_pids()
 	evts_ns2_pid=0
 }
 
-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
-		ip -n $ns mptcp $limits
-	else
-		ip netns exec $ns ./pm_nl_ctl limits $addrs $subflows
-	fi
-}
-
 pm_nl_set_limits()
 {
-	pm_nl_limits "${@}"
+	mptcp_lib_endpoint_ops limits "${@}"
 }
 
 pm_nl_get_limits()
 {
-	pm_nl_limits "${@}"
+	mptcp_lib_endpoint_ops limits "${@}"
 }
 
 pm_nl_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
-		ip -n $ns mptcp endpoint add $addr ${_flags//","/" "} $dev $id $port
-	else
-		ip netns exec $ns ./pm_nl_ctl add $addr $flags $dev $id $port
-	fi
+	mptcp_lib_endpoint_ops add "${@}"
 }
 
 pm_nl_del_endpoint()
 {
-	local ns=$1
-	local id=$2
-	local addr=$3
-
-	if mptcp_lib_is_ip_mptcp; then
-		[ $id -ne 0 ] && addr=''
-		ip -n $ns mptcp endpoint delete id $id $addr
-	else
-		ip netns exec $ns ./pm_nl_ctl del $id $addr
-	fi
+	mptcp_lib_endpoint_ops del "${@}"
 }
 
 pm_nl_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_endpoint_ops flush "${@}"
 }
 
 pm_nl_show_endpoints()
 {
-	local ns=$1
-	local id=$2
-
-	if mptcp_lib_is_ip_mptcp; then
-		local show="show"
-
-		[ -n "$id" ] && show+=" id $id"
-		ip -n $ns mptcp endpoint $show
-	else
-		local dump="dump"
-
-		[ -n "$id" ] && dump="get $id"
-		ip netns exec $ns ./pm_nl_ctl $dump
-	fi
+	mptcp_lib_endpoint_ops show "${@}"
 }
 
 pm_nl_change_endpoint()
 {
-	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"
-	fi
-
-	if mptcp_lib_is_ip_mptcp; then
-		ip -n $ns mptcp endpoint change $addr ${flags//","/" "}
-	else
-		ip netns exec $ns ./pm_nl_ctl set $addr flags $flags
-	fi
+	mptcp_lib_endpoint_ops change "${@}"
 }
 
 pm_nl_check_endpoint()
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() {
+	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
+		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
+		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}
+	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() {
+	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}
+	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() {
+	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}"
+	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
+	"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
+}
-- 
2.40.1

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.