[PATCH mptcp-net 0/5] add helpers and vars in mptcp_lib.sh, part 1

[PATCH mptcp-net 0/5] add helpers and vars in mptcp_lib.sh, part 1

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Part 1 of "add helpers and vars in mptcp_lib.sh", some fixes.

Geliang Tang (5):
  selftests: mptcp: netlink: drop duplicate var ret
  selftests: mptcp: simult flows: define missing vars
  selftests: mptcp: add ss mptcp support checks
  selftests: mptcp: join: change capture/checksum as bool
  selftests: mptcp: diag: change timeout_poll to 30

 tools/testing/selftests/net/mptcp/diag.sh     |  2 +-
 .../selftests/net/mptcp/mptcp_connect.sh      |  6 ++++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 28 +++++++++++--------
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  6 ++++
 .../testing/selftests/net/mptcp/pm_netlink.sh |  1 -
 .../selftests/net/mptcp/simult_flows.sh       |  6 ++++
 6 files changed, 36 insertions(+), 13 deletions(-)

-- 
2.40.1

[PATCH mptcp-net 1/5] selftests: mptcp: netlink: drop duplicate var ret

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The variable 'ret' are defined twice in pm_netlink.sh. This patch drops
this duplicate one.

Fixes: eedbc685321b ("selftests: add PM netlink functional tests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/pm_netlink.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 8f4ff123a7eb..ed1e8380838a 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -28,7 +28,6 @@ sec=$(date +%s)
 rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
 ns1="ns1-$rndh"
 err=$(mktemp)
-ret=0
 
 cleanup()
 {
-- 
2.40.1

[PATCH mptcp-net 2/5] selftests: mptcp: simult flows: define missing vars

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The variables 'large', 'small', 'sout', 'cout', 'capout' and 'size' are
used in multiple functions, so they should be defined as global variables.
This patch redefines them at the beginning of simult_flows.sh.

Fixes: 1a418cb8e888 ("mptcp: simult flow self-tests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/simult_flows.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 619be0e1acf5..3a003d47fccb 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -16,6 +16,12 @@ test_cnt=1
 ret=0
 bail=0
 slack=50
+large=""
+small=""
+sout=""
+cout=""
+capout=""
+size=0
 
 usage() {
 	echo "Usage: $0 [ -b ] [ -c ] [ -d ]"
-- 
2.40.1

Re: [PATCH mptcp-net 2/5] selftests: mptcp: simult flows: define missing vars

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The variables 'large', 'small', 'sout', 'cout', 'capout' and 'size' are
> used in multiple functions, so they should be defined as global variables.
> This patch redefines them at the beginning of simult_flows.sh.
> 
> Fixes: 1a418cb8e888 ("mptcp: simult flow self-tests")

Even if I agree it is better to clearly define global variables at the
beginning, it is not a mistake to define them in a function.

My main point is that it is maybe easier not to consider this as a fix:
it looks like backporting this to stable will create conflicts. They
will be easy to resolve, but still, it will take a bit of time to a few
people. If it doesn't fix an issue, isn't a preparation patch or doesn't
ease the backport of other patches, maybe better to target -next
(without the "Fixes" tag). WDYT?

If there are no other modifications in the series and no objections, no
need to send a new version, I can apply the patch 2 in -next. If you
need to send a v2, please move this patch after all the other fixes of
the series (or as part of a new series).

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

[PATCH mptcp-net 3/5] selftests: mptcp: add ss mptcp support checks

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Commands 'ss -M' are used in scripts mptcp_connect.sh, mptcp_join.sh,
and mptcp_sockopt.sh to display only MPTCP sockets. So it must be checked
if ss tool supports MPTCP in these script.

Fixes: 767389c8dd55 ("selftests: mptcp: dump more info on errors")
Fixes: c2a55e8fd80f ("selftests: mptcp: dump more info on mpjoin errors")
Fixes: dc65fe82fb07 ("selftests: mptcp: add packet mark test case")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 ++++++
 tools/testing/selftests/net/mptcp/mptcp_join.sh    | 6 ++++++
 tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 7898d62fce0b..c49351d85cae 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -154,6 +154,12 @@ if [ $? -ne 0 ];then
 	exit $ksft_skip
 fi
 
+ss -h | grep -q MPTCP
+if [ $? -ne 0 ];then
+	echo "SKIP: ss tool does not support MPTCP"
+	exit $ksft_skip
+fi
+
 sin=$(mktemp)
 sout=$(mktemp)
 cin=$(mktemp)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index c07386e21e0a..5ca1512c3eae 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -161,6 +161,12 @@ check_tools()
 		exit $ksft_skip
 	fi
 
+	ss -h | grep -q MPTCP
+	if [ $? -ne 0 ];then
+		echo "SKIP: ss tool does not support MPTCP"
+		exit $ksft_skip
+	fi
+
 	# Use the legacy version if available to support old kernel versions
 	if iptables-legacy -V &> /dev/null; then
 		iptables="iptables-legacy"
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index c643872ddf47..1b6ecbeda5a1 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -96,6 +96,12 @@ if [ $? -ne 0 ];then
 	exit $ksft_skip
 fi
 
+ss -h | grep -q MPTCP
+if [ $? -ne 0 ];then
+	echo "SKIP: ss tool does not support MPTCP"
+	exit $ksft_skip
+fi
+
 # Use the legacy version if available to support old kernel versions
 if iptables-legacy -V &> /dev/null; then
 	iptables="iptables-legacy"
-- 
2.40.1

Re: [PATCH mptcp-net 3/5] selftests: mptcp: add ss mptcp support checks

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Commands 'ss -M' are used in scripts mptcp_connect.sh, mptcp_join.sh,
> and mptcp_sockopt.sh to display only MPTCP sockets. So it must be checked
> if ss tool supports MPTCP in these script.
> 
> Fixes: 767389c8dd55 ("selftests: mptcp: dump more info on errors")
> Fixes: c2a55e8fd80f ("selftests: mptcp: dump more info on mpjoin errors")
> Fixes: dc65fe82fb07 ("selftests: mptcp: add packet mark test case")

If we only use it to get more debug in these selftests, maybe this check
is not needed? I think it would be better to let other CI running our
tests, and accept we can have less debug info. (Also easier for the
backports if there is only one thing to fix at a time)


I would then add...

 Fixes: e274f7154008 ("selftests: mptcp: add subflow limits test-cases")

... because here it is different, it is used as a feature.

So I think we only need it for mptcp_join.sh, right?

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 6 ++++++
>  tools/testing/selftests/net/mptcp/mptcp_join.sh    | 6 ++++++
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 6 ++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 7898d62fce0b..c49351d85cae 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -154,6 +154,12 @@ if [ $? -ne 0 ];then
>  	exit $ksft_skip
>  fi
>  
> +ss -h | grep -q MPTCP
> +if [ $? -ne 0 ];then
> +	echo "SKIP: ss tool does not support MPTCP"
> +	exit $ksft_skip
> +fi
> +
>  sin=$(mktemp)
>  sout=$(mktemp)
>  cin=$(mktemp)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index c07386e21e0a..5ca1512c3eae 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -161,6 +161,12 @@ check_tools()
>  		exit $ksft_skip
>  	fi
>  
> +	ss -h | grep -q MPTCP
> +	if [ $? -ne 0 ];then

(Even if you might have taken that from diag.sh) This will not pass
shellcheck's validation, I think, right?

Better like this?

  if ! ss -h | grep -q MPTCP; then

(Also, please add a space after ";", like in most of the scripts we have)

Same in the other files (if you need to modify them).

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

[PATCH mptcp-net 4/5] selftests: mptcp: join: change capture/checksum as bool

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To maintain consistency with other scripts, this patch changes vars
'capture' and 'checksum' as bool vars in mptcp_join.

Fixes: b08fbf241064 ("selftests: add test-cases for MPTCP MP_JOIN")
Fixes: af66d3e1c3fa ("selftests: mptcp: enable checksum in mptcp_join.sh")
Fixes: 3c082695e78b ("selftests: mptcp: drop msg argument of chk_csum_nr")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 5ca1512c3eae..b3dc17daf574 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -29,11 +29,11 @@ iptables="iptables"
 ip6tables="ip6tables"
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
-capture=0
-checksum=0
+capture=false
+checksum=false
 ip_mptcp=0
 check_invert=0
-validate_checksum=0
+validate_checksum=false
 init=0
 evts_ns1=""
 evts_ns2=""
@@ -100,7 +100,7 @@ init_partial()
 		ip netns exec $netns sysctl -q net.mptcp.pm_type=0 2>/dev/null || true
 		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
-		if [ $checksum -eq 1 ]; then
+		if $checksum; then
 			ip netns exec $netns sysctl -q net.mptcp.checksum_enabled=1
 		fi
 	done
@@ -386,7 +386,7 @@ reset_with_checksum()
 	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=$ns1_enable
 	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=$ns2_enable
 
-	validate_checksum=1
+	validate_checksum=true
 }
 
 reset_with_allow_join_id0()
@@ -419,7 +419,7 @@ reset_with_allow_join_id0()
 setup_fail_rules()
 {
 	check_invert=1
-	validate_checksum=1
+	validate_checksum=true
 	local i="$1"
 	local ip="${2:-4}"
 	local tables
@@ -1023,7 +1023,7 @@ do_transfer()
 	:> "$sout"
 	:> "$capout"
 
-	if [ $capture -eq 1 ]; then
+	if $capture; then
 		local capuser
 		if [ -z $SUDO_USER ] ; then
 			capuser=""
@@ -1125,7 +1125,7 @@ do_transfer()
 	wait $spid
 	local rets=$?
 
-	if [ $capture -eq 1 ]; then
+	if $capture; then
 	    sleep 1
 	    kill $cappid
 	fi
@@ -1513,7 +1513,7 @@ chk_join_nr()
 	else
 		print_ok
 	fi
-	if [ $validate_checksum -eq 1 ]; then
+	if $validate_checksum; then
 		chk_csum_nr $csum_ns1 $csum_ns2
 		chk_fail_nr $fail_nr $fail_nr
 		chk_rst_nr $rst_nr $rst_nr
@@ -3670,10 +3670,10 @@ while getopts "${all_tests_args}cCih" opt; do
 			tests+=("${all_tests[${opt}]}")
 			;;
 		c)
-			capture=1
+			capture=true
 			;;
 		C)
-			checksum=1
+			checksum=true
 			;;
 		i)
 			ip_mptcp=1
-- 
2.40.1

Re: [PATCH mptcp-net 4/5] selftests: mptcp: join: change capture/checksum as bool

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To maintain consistency with other scripts, this patch changes vars
> 'capture' and 'checksum' as bool vars in mptcp_join.

I understand it is better, but it doesn't really fix a bug, right? Maybe
best not to target -net, no?

> Fixes: b08fbf241064 ("selftests: add test-cases for MPTCP MP_JOIN")
> Fixes: af66d3e1c3fa ("selftests: mptcp: enable checksum in mptcp_join.sh")
> Fixes: 3c082695e78b ("selftests: mptcp: drop msg argument of chk_csum_nr")

... and without the 'Fixes' then

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 5ca1512c3eae..b3dc17daf574 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -29,11 +29,11 @@ iptables="iptables"
>  ip6tables="ip6tables"
>  timeout_poll=30
>  timeout_test=$((timeout_poll * 2 + 1))
> -capture=0
> -checksum=0
> +capture=false
> +checksum=false
>  ip_mptcp=0
>  check_invert=0
> -validate_checksum=0
> +validate_checksum=false
>  init=0

To be honest, I'm not sure if I prefer to use "boolean". At the end, it
is still a string. Instead of comparing values, we now "execute" the
content of the variable, it might not look safe (but it should be).

Also, there are still many other variables using 0/1: ip_mptcp,
check_invert, init. I don't think it is worth changing them.

I'm not against this change for -next, but I'm not sure if it is that
interesting. (But I'm not against using (fake) "boolean" for new code)

WDYT?

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

[PATCH mptcp-net 5/5] selftests: mptcp: diag: change timeout_poll to 30

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In most scripts timeout_poll is defined to 30, but in diag.sh,
it's 100. To maintain consistency with other scripts, this patch
changes it to 30.

Fixes: 5888a61cb4e0 ("selftests: mptcp: launch mptcp_connect with timeout")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 04fcb8a077c9..3505c98be127 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -8,7 +8,7 @@ rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
 ns="ns1-$rndh"
 ksft_skip=4
 test_cnt=1
-timeout_poll=100
+timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
 ret=0
 
-- 
2.40.1

Re: selftests: mptcp: diag: change timeout_poll to 30: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7881714529

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c502a99a8f4a


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: selftests: mptcp: diag: change timeout_poll to 30: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI (Cirrus) did some validations with a debug kernel and here is its report:

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6220816166158336
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6220816166158336/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4813441282605056
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4813441282605056/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/c502a99a8f4a


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-debug

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-net 5/5] selftests: mptcp: diag: change timeout_poll to 30

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In most scripts timeout_poll is defined to 30, but in diag.sh,
> it's 100. To maintain consistency with other scripts, this patch
> changes it to 30.

It looks like we use 100 (mptcp_connect (...) -t 100) from the
beginning, see commit df62f2ec3df6 ("selftests/mptcp: add diag interface
tests").

It makes sense to align this with the others, but it would be good to
explain it is OK to do that in the commit message, something like:

  Even if it is set to 100ms from the beginning with
  commit df62f2ec3df6 ("selftests/mptcp: add diag interface tests"),
  there is no reason not to have it to 30ms like all the other tests.
  diag.sh is not supposed to be slower than the other ones.

Or similar.

> Fixes: 5888a61cb4e0 ("selftests: mptcp: launch mptcp_connect with timeout")

Here as well, it is not really a fix, not need to do the effort to
backport that, and it changes the behaviour (a bit). I think it is
better to target -next, no?

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

Re: [PATCH mptcp-net 0/5] add helpers and vars in mptcp_lib.sh, part 1

[Matthieu Baerts]

Hi Geliang,

On 13/02/2024 05:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Part 1 of "add helpers and vars in mptcp_lib.sh", some fixes.
> 
> Geliang Tang (5):
>   selftests: mptcp: netlink: drop duplicate var ret
>   selftests: mptcp: simult flows: define missing vars
>   selftests: mptcp: add ss mptcp support checks
>   selftests: mptcp: join: change capture/checksum as bool
>   selftests: mptcp: diag: change timeout_poll to 30

Thank you for the new series!

I think only patches 1/5 and 3/5 are for -net. I also have some comments
in some of them. Please check the individual patches.

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