[PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh

[PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This set uses the public helpers defined in net selftests to replace
helpers in mptcp_lib.sh.

v2:
 - only patch 3 updated, mptcp_join.sh, move cleanup_all_ns from
   cleanup_partial to cleanup, setup_ns will delete existing
   namespaces automaticly.
 - update commit logs.

Geliang Tang (4):
  selftests: mptcp: rename ns to ns1 in diag.sh
  selftests: mptcp: use setup_ns helper in lib.sh
  selftests: mptcp: use cleanup_all_ns helper in lib.sh
  selftests: mptcp: use wait_local_port_listen helper

 tools/testing/selftests/net/mptcp/diag.sh     | 58 +++++++++----------
 .../selftests/net/mptcp/mptcp_connect.sh      |  3 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh |  3 +-
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 43 +++-----------
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  2 +-
 .../testing/selftests/net/mptcp/pm_netlink.sh |  2 +-
 .../selftests/net/mptcp/simult_flows.sh       |  2 +-
 .../selftests/net/mptcp/userspace_pm.sh       |  2 +-
 8 files changed, 44 insertions(+), 71 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v2 1/4] selftests: mptcp: rename ns to ns1 in diag.sh

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The next commit will use setup_ns() helper defined in lib.sh instead
of mptcp helper mptcp_lib_ns_init(). But the namespace "ns" in diag.sh
makes it not working. Since "ns" is a local variable in this helper.
So this patch renames it as "ns1".

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh | 58 +++++++++++------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 776d43a6922d..eec1f04d231f 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -8,7 +8,7 @@
 
 . "$(dirname "${0}")/mptcp_lib.sh"
 
-ns=""
+ns1=""
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
 ret=0
@@ -19,10 +19,10 @@ flush_pids()
 	# give it some time
 	sleep 1.1
 
-	ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGUSR1 &>/dev/null
+	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGUSR1 &>/dev/null
 
 	for _ in $(seq $((timeout_poll * 10))); do
-		[ -z "$(ip netns pids "${ns}")" ] && break
+		[ -z "$(ip netns pids "${ns1}")" ] && break
 		sleep 0.1
 	done
 }
@@ -31,9 +31,9 @@ flush_pids()
 #shellcheck disable=SC2317
 cleanup()
 {
-	ip netns pids "${ns}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
+	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
 
-	mptcp_lib_ns_exit "${ns}"
+	mptcp_lib_ns_exit "${ns1}"
 }
 
 mptcp_lib_check_mptcp
@@ -41,7 +41,7 @@ mptcp_lib_check_tools ip ss
 
 get_msk_inuse()
 {
-	ip netns exec $ns cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
+	ip netns exec $ns1 cat /proc/net/protocols | awk '$1~/^MPTCP$/{print $3}'
 }
 
 __chk_nr()
@@ -75,7 +75,7 @@ __chk_msk_nr()
 	local condition=$1
 	shift 1
 
-	__chk_nr "ss -inmHMN $ns | $condition" "$@"
+	__chk_nr "ss -inmHMN $ns1 | $condition" "$@"
 }
 
 chk_msk_nr()
@@ -88,8 +88,8 @@ chk_listener_nr()
 	local expected=$1
 	local msg="$2"
 
-	__chk_nr "ss -nlHMON $ns | wc -l" "$expected" "$msg - mptcp" 0
-	__chk_nr "ss -nlHtON $ns | wc -l" "$expected" "$msg - subflows"
+	__chk_nr "ss -nlHMON $ns1 | wc -l" "$expected" "$msg - mptcp" 0
+	__chk_nr "ss -nlHtON $ns1 | wc -l" "$expected" "$msg - subflows"
 }
 
 wait_msk_nr()
@@ -105,7 +105,7 @@ wait_msk_nr()
 	msg=$*
 
 	while [ $i -lt $timeout ]; do
-		nr=$(ss -inmHMN $ns | $condition)
+		nr=$(ss -inmHMN $ns1 | $condition)
 		[ $nr == $expected ] && break;
 		[ $nr -gt $max ] && max=$nr
 		i=$((i + 1))
@@ -143,7 +143,7 @@ __chk_listen()
 	local expected=$2
 	local msg="$3"
 
-	__chk_nr "ss -N $ns -Ml '$filter' | grep -c LISTEN" "$expected" "$msg" 0
+	__chk_nr "ss -N $ns1 -Ml '$filter' | grep -c LISTEN" "$expected" "$msg" 0
 }
 
 chk_msk_listen()
@@ -173,7 +173,7 @@ chk_msk_inuse()
 		msg+=" after flush"
 	fi
 
-	listen_nr=$(ss -N "${ns}" -Ml | grep -c LISTEN)
+	listen_nr=$(ss -N "${ns1}" -Ml | grep -c LISTEN)
 	expected=$((expected + listen_nr))
 
 	for _ in $(seq 10); do
@@ -196,7 +196,7 @@ chk_msk_cestab()
 		msg+=" after flush"
 	fi
 
-	__chk_nr "mptcp_lib_get_counter ${ns} MPTcpExtMPCurrEstab" \
+	__chk_nr "mptcp_lib_get_counter ${ns1} MPTcpExtMPCurrEstab" \
 		 "${expected}" "${msg}" ""
 }
 
@@ -205,7 +205,7 @@ msk_info_get_value()
 	local port="${1}"
 	local info="${2}"
 
-	ss -N "${ns}" -inHM dport "${port}" | \
+	ss -N "${ns1}" -inHM dport "${port}" | \
 		mptcp_lib_get_info_value "${info}" "${info}"
 }
 
@@ -267,23 +267,23 @@ wait_connected()
 }
 
 trap cleanup EXIT
-mptcp_lib_ns_init ns
+mptcp_lib_ns_init ns1
 
 echo "a" | \
 	timeout ${timeout_test} \
-		ip netns exec $ns \
+		ip netns exec $ns1 \
 			./mptcp_connect -p 10000 -l -t ${timeout_poll} -w 20 \
 				0.0.0.0 >/dev/null &
-mptcp_lib_wait_local_port_listen $ns 10000
+mptcp_lib_wait_local_port_listen $ns1 10000
 chk_msk_nr 0 "no msk on netns creation"
 chk_msk_listen 10000
 
 echo "b" | \
 	timeout ${timeout_test} \
-		ip netns exec $ns \
+		ip netns exec $ns1 \
 			./mptcp_connect -p 10000 -r 0 -t ${timeout_poll} -w 20 \
 				127.0.0.1 >/dev/null &
-wait_connected $ns 10000
+wait_connected $ns1 10000
 chk_msk_nr 2 "after MPC handshake "
 chk_last_time_info 10000
 chk_msk_remote_key_nr 2 "....chk remote_key"
@@ -297,16 +297,16 @@ chk_msk_cestab 0 "2->0"
 
 echo "a" | \
 	timeout ${timeout_test} \
-		ip netns exec $ns \
+		ip netns exec $ns1 \
 			./mptcp_connect -p 10001 -l -s TCP -t ${timeout_poll} -w 20 \
 				0.0.0.0 >/dev/null &
-mptcp_lib_wait_local_port_listen $ns 10001
+mptcp_lib_wait_local_port_listen $ns1 10001
 echo "b" | \
 	timeout ${timeout_test} \
-		ip netns exec $ns \
+		ip netns exec $ns1 \
 			./mptcp_connect -p 10001 -r 0 -t ${timeout_poll} -w 20 \
 				127.0.0.1 >/dev/null &
-wait_connected $ns 10001
+wait_connected $ns1 10001
 chk_msk_fallback_nr 1 "check fallback"
 chk_msk_inuse 1
 chk_msk_cestab 1
@@ -319,16 +319,16 @@ NR_CLIENTS=100
 for I in $(seq 1 $NR_CLIENTS); do
 	echo "a" | \
 		timeout ${timeout_test} \
-			ip netns exec $ns \
+			ip netns exec $ns1 \
 				./mptcp_connect -p $((I+10001)) -l -w 20 \
 					-t ${timeout_poll} 0.0.0.0 >/dev/null &
 done
-mptcp_lib_wait_local_port_listen $ns $((NR_CLIENTS + 10001))
+mptcp_lib_wait_local_port_listen $ns1 $((NR_CLIENTS + 10001))
 
 for I in $(seq 1 $NR_CLIENTS); do
 	echo "b" | \
 		timeout ${timeout_test} \
-			ip netns exec $ns \
+			ip netns exec $ns1 \
 				./mptcp_connect -p $((I+10001)) -w 20 \
 					-t ${timeout_poll} 127.0.0.1 >/dev/null &
 done
@@ -344,16 +344,16 @@ chk_msk_cestab 0 "many->0"
 chk_listener_nr 0 "no listener sockets"
 NR_SERVERS=100
 for I in $(seq 1 $NR_SERVERS); do
-	ip netns exec $ns ./mptcp_connect -p $((I + 20001)) \
+	ip netns exec $ns1 ./mptcp_connect -p $((I + 20001)) \
 		-t ${timeout_poll} -l 0.0.0.0 >/dev/null 2>&1 &
 done
-mptcp_lib_wait_local_port_listen $ns $((NR_SERVERS + 20001))
+mptcp_lib_wait_local_port_listen $ns1 $((NR_SERVERS + 20001))
 
 chk_listener_nr $NR_SERVERS "many listener sockets"
 
 # graceful termination
 for I in $(seq 1 $NR_SERVERS); do
-	echo a | ip netns exec $ns ./mptcp_connect -p $((I + 20001)) 127.0.0.1 >/dev/null 2>&1 &
+	echo a | ip netns exec $ns1 ./mptcp_connect -p $((I + 20001)) 127.0.0.1 >/dev/null 2>&1 &
 done
 flush_pids
 
-- 
2.43.0

Re: [PATCH mptcp-next v2 1/4] selftests: mptcp: rename ns to ns1 in diag.sh

[Matthieu Baerts]

Hi Geliang,

On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The next commit will use setup_ns() helper defined in lib.sh instead
> of mptcp helper mptcp_lib_ns_init(). But the namespace "ns" in diag.sh
> makes it not working. Since "ns" is a local variable in this helper.
> So this patch renames it as "ns1".

Can we not modify 'setup_ns' from 'lib.sh' to avoid the conflict
instead? e.g. using the variable '_ns' or '__netns' instead of 'ns'.

By doing that, we don't have to modify diag.sh a bit everywhere, which
is annoying for the backports, etc.

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

[PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() helper
defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then for
each namespace in NS_LIST, run all sysctl commands. This can drop some
duplicate code.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index ad2ebda5cb64..59eb77e7813d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -1,6 +1,8 @@
 #! /bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
+. "$(dirname "${0}")/../lib.sh"
+
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
 readonly KSFT_SKIP=4
@@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
 }
 
 mptcp_lib_ns_init() {
-	local sec rndh
-
-	sec=$(date +%s)
-	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
+	setup_ns "${@}"
 
 	local netns
-	for netns in "${@}"; do
-		eval "${netns}=${netns}-${rndh}"
-
-		ip netns add "${!netns}" || exit ${KSFT_SKIP}
-		ip -net "${!netns}" link set lo up
-		ip netns exec "${!netns}" sysctl -q net.mptcp.enabled=1
-		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
-		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
+	for netns in $NS_LIST; do
+		ip netns exec "${netns}" sysctl -q net.mptcp.enabled=1
+		ip netns exec "${netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
+		ip netns exec "${netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
 	done
 }
 
-- 
2.43.0

Re: [PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh

[Matthieu Baerts]

Hi Geliang,

On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() helper
> defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then for
> each namespace in NS_LIST, run all sysctl commands. This can drop some
> duplicate code.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index ad2ebda5cb64..59eb77e7813d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh


(...)

> @@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
>  }
>  
>  mptcp_lib_ns_init() {
> -	local sec rndh
> -
> -	sec=$(date +%s)
> -	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
> +	setup_ns "${@}"
>  
>  	local netns
> -	for netns in "${@}"; do
> -		eval "${netns}=${netns}-${rndh}"
> -
> -		ip netns add "${!netns}" || exit ${KSFT_SKIP}
> -		ip -net "${!netns}" link set lo up
> -		ip netns exec "${!netns}" sysctl -q net.mptcp.enabled=1
> -		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
> -		ip netns exec "${!netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
> +	for netns in $NS_LIST; do

Could you add {} (${NS_LIST}), to keep the same style (and usually
recommended in order to avoid typos, etc.)

Also, I wonder if it is a good idea to use '${NS_LIST}': it might
contain existing netns. e.g. with your patch 3/4, in mptcp_connect,
mptcp_lib_ns_init will be called twice, modifying the first netns twice.

What if you keep:

  for netns in "${@}"; do

and use "${!netns}" instead?


> +		ip netns exec "${netns}" sysctl -q net.mptcp.enabled=1
> +		ip netns exec "${netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
> +		ip netns exec "${netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
>  	done
>  }
>  

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

Re: [PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh

[Geliang Tang]

Hi Matt,

On Thu, 2024-05-23 at 11:18 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 23/05/2024 10:08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch includes lib.sh into mptcp_lib.sh, uses setup_ns()
> > helper
> > defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then
> > for
> > each namespace in NS_LIST, run all sysctl commands. This can drop
> > some
> > duplicate code.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 19 +++++++--------
> > ----
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > index ad2ebda5cb64..59eb77e7813d 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> 
> 
> (...)
> 
> > @@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
> >  }
> >  
> >  mptcp_lib_ns_init() {
> > -	local sec rndh
> > -
> > -	sec=$(date +%s)
> > -	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
> > +	setup_ns "${@}"
> >  
> >  	local netns
> > -	for netns in "${@}"; do
> > -		eval "${netns}=${netns}-${rndh}"
> > -
> > -		ip netns add "${!netns}" || exit ${KSFT_SKIP}
> > -		ip -net "${!netns}" link set lo up
> > -		ip netns exec "${!netns}" sysctl -q
> > net.mptcp.enabled=1
> > -		ip netns exec "${!netns}" sysctl -q
> > net.ipv4.conf.all.rp_filter=0
> > -		ip netns exec "${!netns}" sysctl -q
> > net.ipv4.conf.default.rp_filter=0
> > +	for netns in $NS_LIST; do
> 
> Could you add {} (${NS_LIST}), to keep the same style (and usually
> recommended in order to avoid typos, etc.)
> 
> Also, I wonder if it is a good idea to use '${NS_LIST}': it might
> contain existing netns. e.g. with your patch 3/4, in mptcp_connect,
> mptcp_lib_ns_init will be called twice, modifying the first netns
> twice.
> 
> What if you keep:
> 
>   for netns in "${@}"; do
> 
> and use "${!netns}" instead?

We don't know the name of ns now because "mktemp -u XXXXXX" is executed
within setup_ns(). So we have to use $NS_LIST.

> 
> 
> > +		ip netns exec "${netns}" sysctl -q
> > net.mptcp.enabled=1
> > +		ip netns exec "${netns}" sysctl -q
> > net.ipv4.conf.all.rp_filter=0
> > +		ip netns exec "${netns}" sysctl -q
> > net.ipv4.conf.default.rp_filter=0
> >  	done
> >  }
> >  
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh

[Matthieu Baerts]

On 23/05/2024 11:26, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, 2024-05-23 at 11:18 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 23/05/2024 10:08, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch includes lib.sh into mptcp_lib.sh, uses setup_ns()
>>> helper
>>> defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then
>>> for
>>> each namespace in NS_LIST, run all sysctl commands. This can drop
>>> some
>>> duplicate code.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 19 +++++++--------
>>> ----
>>>  1 file changed, 7 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> index ad2ebda5cb64..59eb77e7813d 100644
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>
>>
>> (...)
>>
>>> @@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
>>>  }
>>>  
>>>  mptcp_lib_ns_init() {
>>> -	local sec rndh
>>> -
>>> -	sec=$(date +%s)
>>> -	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
>>> +	setup_ns "${@}"
>>>  
>>>  	local netns
>>> -	for netns in "${@}"; do
>>> -		eval "${netns}=${netns}-${rndh}"
>>> -
>>> -		ip netns add "${!netns}" || exit ${KSFT_SKIP}
>>> -		ip -net "${!netns}" link set lo up
>>> -		ip netns exec "${!netns}" sysctl -q
>>> net.mptcp.enabled=1
>>> -		ip netns exec "${!netns}" sysctl -q
>>> net.ipv4.conf.all.rp_filter=0
>>> -		ip netns exec "${!netns}" sysctl -q
>>> net.ipv4.conf.default.rp_filter=0
>>> +	for netns in $NS_LIST; do
>>
>> Could you add {} (${NS_LIST}), to keep the same style (and usually
>> recommended in order to avoid typos, etc.)
>>
>> Also, I wonder if it is a good idea to use '${NS_LIST}': it might
>> contain existing netns. e.g. with your patch 3/4, in mptcp_connect,
>> mptcp_lib_ns_init will be called twice, modifying the first netns
>> twice.
>>
>> What if you keep:
>>
>>   for netns in "${@}"; do
>>
>> and use "${!netns}" instead?
> 
> We don't know the name of ns now because "mktemp -u XXXXXX" is executed
> within setup_ns(). So we have to use $NS_LIST.

Will 'setup_ns' not assign the variable names given in parameter? If we
give 'ns1', it will set 'ns1=xxxx'. So we can use '${ns1}' to get the
name. If 'netns="ns1"', we can then get the name with '${!netns}', using
'!' (${!netns}) to resolve the name, no?

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

[PATCH mptcp-next v2 3/4] selftests: mptcp: use cleanup_all_ns helper in lib.sh

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch uses cleanup_all_ns() helper defined in lib.sh instead of
all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
mptcp helper in mptcp_lib.sh.

In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
cleanup(), together with "ns1 - ns4".

In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial() directly,
each existing namespace will delete automaticly in setup_ns(), only
adding cleanup_all_ns in cleanup() is enough.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh          | 2 +-
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +--
 tools/testing/selftests/net/mptcp/mptcp_join.sh    | 3 +--
 tools/testing/selftests/net/mptcp/mptcp_lib.sh     | 8 --------
 tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 2 +-
 tools/testing/selftests/net/mptcp/pm_netlink.sh    | 2 +-
 tools/testing/selftests/net/mptcp/simult_flows.sh  | 2 +-
 tools/testing/selftests/net/mptcp/userspace_pm.sh  | 2 +-
 8 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index eec1f04d231f..9e19e3e8d833 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -33,7 +33,7 @@ cleanup()
 {
 	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -SIGKILL &>/dev/null
 
-	mptcp_lib_ns_exit "${ns1}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index b77fb7065bfb..4e2c5dd0de3c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -142,7 +142,7 @@ cleanup()
 	rm -f "$sin" "$sout"
 	rm -f "$capout"
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
@@ -271,7 +271,6 @@ check_mptcp_disabled()
 	local err=0
 	LC_ALL=C ip netns exec ${disabled_ns} ./mptcp_connect -p 10000 -s MPTCP 127.0.0.1 < "$cin" 2>&1 | \
 		grep -q "^socket: Protocol not available$" && err=1
-	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
 		mptcp_lib_pr_fail "New MPTCP socket cannot be blocked via sysctl"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fefa9173bdaa..87a518b8c19f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -132,8 +132,6 @@ init_shapers()
 cleanup_partial()
 {
 	rm -f "$capout"
-
-	mptcp_lib_ns_exit "${ns1}" "${ns2}"
 }
 
 init() {
@@ -166,6 +164,7 @@ cleanup()
 	rm -rf $evts_ns1 $evts_ns2
 	rm -f "$err"
 	cleanup_partial
+	cleanup_all_ns
 }
 
 print_check()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 59eb77e7813d..bd7d78e4aa83 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
 	done
 }
 
-mptcp_lib_ns_exit() {
-	local netns
-	for netns in "${@}"; do
-		ip netns del "${netns}"
-		rm -f /tmp/"${netns}".{nstat,out}
-	done
-}
-
 mptcp_lib_events() {
 	local ns="${1}"
 	local evts="${2}"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 68899a303a1a..e1026b028739 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -98,7 +98,7 @@ init()
 #shellcheck disable=SC2317
 cleanup()
 {
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
+	cleanup_all_ns
 	rm -f "$cin" "$cout"
 	rm -f "$sin" "$sout"
 }
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 2757378b1b13..5b4d83c2e280 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -36,7 +36,7 @@ err=$(mktemp)
 cleanup()
 {
 	rm -f "${err}"
-	mptcp_lib_ns_exit "${ns1}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index d0b39c2e38a3..6eddb3bba2e8 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -42,7 +42,7 @@ cleanup()
 	rm -f "$large" "$small"
 	rm -f "$capout"
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
+	cleanup_all_ns
 }
 
 mptcp_lib_check_mptcp
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 9e2981f2d7f5..0c089e7f5f0a 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -107,7 +107,7 @@ cleanup()
 		mptcp_lib_kill_wait $pid
 	done
 
-	mptcp_lib_ns_exit "${ns1}" "${ns2}"
+	cleanup_all_ns
 
 	rm -rf $file $client_evts $server_evts
 
-- 
2.43.0

Re: [PATCH mptcp-next v2 3/4] selftests: mptcp: use cleanup_all_ns helper in lib.sh

[Geliang Tang]

On Thu, 2024-05-23 at 16:08 +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch uses cleanup_all_ns() helper defined in lib.sh instead of
> all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
> mptcp helper in mptcp_lib.sh.
> 
> In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
> directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
> cleanup(), together with "ns1 - ns4".
> 
> In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial()
> directly,
> each existing namespace will delete automaticly in setup_ns(), only

Sorry, should be "automatically", CI complain about it.

-Geliang

> adding cleanup_all_ns in cleanup() is enough.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh          | 2 +-
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 3 +--
>  tools/testing/selftests/net/mptcp/mptcp_join.sh    | 3 +--
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh     | 8 --------
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 2 +-
>  tools/testing/selftests/net/mptcp/pm_netlink.sh    | 2 +-
>  tools/testing/selftests/net/mptcp/simult_flows.sh  | 2 +-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh  | 2 +-
>  8 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh
> b/tools/testing/selftests/net/mptcp/diag.sh
> index eec1f04d231f..9e19e3e8d833 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -33,7 +33,7 @@ cleanup()
>  {
>  	ip netns pids "${ns1}" | xargs --no-run-if-empty kill -
> SIGKILL &>/dev/null
>  
> -	mptcp_lib_ns_exit "${ns1}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index b77fb7065bfb..4e2c5dd0de3c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -142,7 +142,7 @@ cleanup()
>  	rm -f "$sin" "$sout"
>  	rm -f "$capout"
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}" "${ns4}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> @@ -271,7 +271,6 @@ check_mptcp_disabled()
>  	local err=0
>  	LC_ALL=C ip netns exec ${disabled_ns} ./mptcp_connect -p
> 10000 -s MPTCP 127.0.0.1 < "$cin" 2>&1 | \
>  		grep -q "^socket: Protocol not available$" && err=1
> -	mptcp_lib_ns_exit "${disabled_ns}"
>  
>  	if [ ${err} -eq 0 ]; then
>  		mptcp_lib_pr_fail "New MPTCP socket cannot be
> blocked via sysctl"
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fefa9173bdaa..87a518b8c19f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -132,8 +132,6 @@ init_shapers()
>  cleanup_partial()
>  {
>  	rm -f "$capout"
> -
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}"
>  }
>  
>  init() {
> @@ -166,6 +164,7 @@ cleanup()
>  	rm -rf $evts_ns1 $evts_ns2
>  	rm -f "$err"
>  	cleanup_partial
> +	cleanup_all_ns
>  }
>  
>  print_check()
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 59eb77e7813d..bd7d78e4aa83 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
>  	done
>  }
>  
> -mptcp_lib_ns_exit() {
> -	local netns
> -	for netns in "${@}"; do
> -		ip netns del "${netns}"
> -		rm -f /tmp/"${netns}".{nstat,out}
> -	done
> -}
> -
>  mptcp_lib_events() {
>  	local ns="${1}"
>  	local evts="${2}"
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 68899a303a1a..e1026b028739 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -98,7 +98,7 @@ init()
>  #shellcheck disable=SC2317
>  cleanup()
>  {
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns_sbox}"
> +	cleanup_all_ns
>  	rm -f "$cin" "$cout"
>  	rm -f "$sin" "$sout"
>  }
> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 2757378b1b13..5b4d83c2e280 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -36,7 +36,7 @@ err=$(mktemp)
>  cleanup()
>  {
>  	rm -f "${err}"
> -	mptcp_lib_ns_exit "${ns1}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh
> b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index d0b39c2e38a3..6eddb3bba2e8 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -42,7 +42,7 @@ cleanup()
>  	rm -f "$large" "$small"
>  	rm -f "$capout"
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}" "${ns3}"
> +	cleanup_all_ns
>  }
>  
>  mptcp_lib_check_mptcp
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 9e2981f2d7f5..0c089e7f5f0a 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -107,7 +107,7 @@ cleanup()
>  		mptcp_lib_kill_wait $pid
>  	done
>  
> -	mptcp_lib_ns_exit "${ns1}" "${ns2}"
> +	cleanup_all_ns
>  
>  	rm -rf $file $client_evts $server_evts
>  

Re: [PATCH mptcp-next v2 3/4] selftests: mptcp: use cleanup_all_ns helper in lib.sh

[Matthieu Baerts]

Hi Geliang,

On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch uses cleanup_all_ns() helper defined in lib.sh instead of
> all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
> mptcp helper in mptcp_lib.sh.
> 
> In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
> directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
> cleanup(), together with "ns1 - ns4".

Mmh, I think it is better to clear resources when we don't need them,
than cleaning them all at the end. New code doing that might be OK, but
here switching to that while not really gaining anything new looks less
good.

> In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial() directly,
> each existing namespace will delete automaticly in setup_ns(), only
> adding cleanup_all_ns in cleanup() is enough.

Mmh, same here: I think it is better to clean at the end of a test, than
at the beginning of a new one: if there are issues to delete it, it will
be clearer it is due to the previous test, not the new one.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 59eb77e7813d..bd7d78e4aa83 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
>  	done
>  }
>  
> -mptcp_lib_ns_exit() {
> -	local netns
> -	for netns in "${@}"; do
> -		ip netns del "${netns}"
> -		rm -f /tmp/"${netns}".{nstat,out}

These files are no longer removed. It is then no longer just a "simple"
refactoring. Such behaviour change should be mentioned in the commit
message, explaining why it is OK to do that.

I would suggest to keep mptcp_lib_ns_exit helper(), and simply call
'cleanup_ns "${@}"' here instead. If you do that, there is no need to
modify the other selftests, and you can squash this in patch 2 with
'setup_ns()'. WDYT?

> -	done
> -}
> -
>  mptcp_lib_events() {
>  	local ns="${1}"
>  	local evts="${2}"

(...)

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

[PATCH mptcp-next v2 4/4] selftests: mptcp: use wait_local_port_listen helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch includes net_helper.sh into mptcp_lib.sh, uses the helper
wait_local_port_listen() defined in it to implement the similar mptcp
helper. This can drop some duplicate code.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index bd7d78e4aa83..e039c88a64ed 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 . "$(dirname "${0}")/../lib.sh"
+. "$(dirname "${0}")/../net_helper.sh"
 
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
@@ -337,20 +338,7 @@ mptcp_lib_check_transfer() {
 
 # $1: ns, $2: port
 mptcp_lib_wait_local_port_listen() {
-	local listener_ns="${1}"
-	local port="${2}"
-
-	local port_hex
-	port_hex="$(printf "%04X" "${port}")"
-
-	local _
-	for _ in $(seq 10); do
-		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
-			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) \
-			     {rc=0; exit}} END {exit rc}" &&
-			break
-		sleep 0.1
-	done
+	wait_local_port_listen "${@}" "tcp"
 }
 
 mptcp_lib_check_output() {
-- 
2.43.0

Re: [PATCH mptcp-next v2 4/4] selftests: mptcp: use wait_local_port_listen helper

[Matthieu Baerts]

Hi Geliang,

On 23/05/2024 10:09, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch includes net_helper.sh into mptcp_lib.sh, uses the helper
> wait_local_port_listen() defined in it to implement the similar mptcp
> helper. This can drop some duplicate code.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index bd7d78e4aa83..e039c88a64ed 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh

(...)

> @@ -337,20 +338,7 @@ mptcp_lib_check_transfer() {
>  
>  # $1: ns, $2: port
>  mptcp_lib_wait_local_port_listen() {
> -	local listener_ns="${1}"
> -	local port="${2}"
> -
> -	local port_hex
> -	port_hex="$(printf "%04X" "${port}")"
> -
> -	local _
> -	for _ in $(seq 10); do
> -		ip netns exec "${listener_ns}" cat /proc/net/tcp* | \
> -			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/ && \$4 ~ /0A/) \
> -			     {rc=0; exit}} END {exit rc}" &&
> -			break
> -		sleep 0.1
> -	done
> +	wait_local_port_listen "${@}" "tcp"

That's a shame people who duplicated the code didn't do that for us (or
at least they could have mentioned where the code was coming from)...

Anyway, good you saw that, and good idea to use it.

>  }
>  
>  mptcp_lib_check_output() {

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

Re: [PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/9204660898

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/4dfead4c10f4
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=855244


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh

[Matthieu Baerts]

Hi Geliang,

On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This set uses the public helpers defined in net selftests to replace
> helpers in mptcp_lib.sh.
> 
> v2:
>  - only patch 3 updated, mptcp_join.sh, move cleanup_all_ns from
>    cleanup_partial to cleanup, setup_ns will delete existing
>    namespaces automaticly.
>  - update commit logs.

Thank you for the modifications! Good idea to use the main lib.sh file.

I think there are some modifications we can avoid here, and only modify
the lib files, see my comments in the individual patches.

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