[PATCH mptcp-next v7 0/8] add helpers and vars in mptcp_lib.sh, part 3

[PATCH mptcp-next v7 0/8] add helpers and vars in mptcp_lib.sh, part 3

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v7:
 - address every comment from Matt in v6.

v6:
 - only include the first 7 patches in v5
 - add mptcp_lib_pr_* helpers
 - add print_title helpers
 - align the output of simult_flows.sh

v5:
 - don't use mptcp_lib_print_test_counter in mptcp_join.sh, it breaks
   skip_test().

v4:
 - set test_cnt to 0, use ++counter in mptcp_lib_print_test_counter() to
   fix the following mismatched test counters:

# 012 userspace pm server fullmesh
#       syn                                 [ OK ]
#       synack                              [ OK ]
#       ack                                 [ OK ]
#       add                                 [ OK ]
#       echo                                [FAIL] got 1 ADD_ADDR echo[s] expected 2
# Server ns stats
# TcpPassiveOpens                 5                  0.0
# TcpInSegs                       25                 0.0
... ...
#
# 1 failure(s) has(ve) been detected:
#       - 13: userspace pm server fullmesh

v3:
 - fix shellcheck errors in v2

v2:
 - fix shellcheck errors in v1
 - print test results with counters

Geliang Tang (8):
  selftests: mptcp: diag: return KSFT_FAIL not test_cnt
  selftests: mptcp: connect: add dedicated port counter
  selftests: mptcp: connect: fix misaligned output
  selftests: mptcp: simult_flows: fix misaligned output
  selftests: mptcp: sockopt: unify ipv4/ipv6 as v4/v6
  selftests: mptcp: add test counter helpers
  selftests: mptcp: print test results with counters
  selftests: mptcp: print test results with colors

 tools/testing/selftests/net/mptcp/diag.sh     | 25 +++----
 .../selftests/net/mptcp/mptcp_connect.sh      | 74 ++++++++++---------
 .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++---
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 57 +++++++++++---
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 37 +++++-----
 .../testing/selftests/net/mptcp/pm_netlink.sh |  5 +-
 .../selftests/net/mptcp/simult_flows.sh       | 20 +++--
 .../selftests/net/mptcp/userspace_pm.sh       | 21 ++----
 8 files changed, 149 insertions(+), 112 deletions(-)

-- 
2.40.1

[PATCH mptcp-net v7 1/8] selftests: mptcp: diag: return KSFT_FAIL not test_cnt

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The test counter 'test_cnt' should not be returned in diag.sh, e.g. what
if only the 4th test fail? Will do 'exit 4' which is 'exit ${KSFT_SKIP}',
the whole test will be marked as skipped instead of 'failed'!

So we should do ret=${KSFT_FAIL} instead.

Fixes: df62f2ec3df6 ("selftests/mptcp: add diag interface tests")
Fixes: 42fb6cddec3b ("selftests: mptcp: more stable diag tests")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index d8f080f934ac..ff02b699840d 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -63,7 +63,7 @@ __chk_nr()
 		else
 			echo "[ fail ] expected $expected found $nr"
 			mptcp_lib_result_fail "${msg}"
-			ret=$test_cnt
+			ret=${KSFT_FAIL}
 		fi
 	else
 		echo "[  ok  ]"
@@ -118,11 +118,11 @@ wait_msk_nr()
 	if [ $i -ge $timeout ]; then
 		echo "[ fail ] timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
-		ret=$test_cnt
+		ret=${KSFT_FAIL}
 	elif [ $nr != $expected ]; then
 		echo "[ fail ] expected $expected found $nr"
 		mptcp_lib_result_fail "${msg} # unexpected result"
-		ret=$test_cnt
+		ret=${KSFT_FAIL}
 	else
 		echo "[  ok  ]"
 		mptcp_lib_result_pass "${msg}"
-- 
2.40.1

Re: [PATCH mptcp-net v7 1/8] selftests: mptcp: diag: return KSFT_FAIL not test_cnt

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The test counter 'test_cnt' should not be returned in diag.sh, e.g. what
> if only the 4th test fail? Will do 'exit 4' which is 'exit ${KSFT_SKIP}',
> the whole test will be marked as skipped instead of 'failed'!
> 
> So we should do ret=${KSFT_FAIL} instead.

Thank you for this fix. It looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

And applied in our tree (fix for -net):

New patches for t/upstream-net and t/upstream:
- 5adbc5b033e75: selftests: mptcp: diag: return KSFT_FAIL not test_cnt
- Results: c618a340fe88b..0f15573ab7d6f (export-net)
- Results: ace42d7d3a0ff..80f7cbd34537d (export)

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

[PATCH mptcp-next v7 2/8] selftests: mptcp: connect: add dedicated port counter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
increase port numbers in mptcp_connect.sh.

This can avoid outputting discontinuous test counters.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 0ca2960c9099..4f40221a86cb 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -33,6 +33,7 @@ do_tcp=0
 checksum=false
 filesize=0
 connect_per_transfer=1
+PORT=0
 
 if [ $tc_loss -eq 100 ];then
 	tc_loss=1%
@@ -314,7 +315,7 @@ do_transfer()
 	local extra_args="$7"
 
 	local port
-	port=$((10000+TEST_COUNT))
+	port=$((10000+PORT++))
 	TEST_COUNT=$((TEST_COUNT+1))
 
 	if [ "$rcvbuf" -gt 0 ]; then
@@ -710,7 +711,7 @@ EOF
 
 	echo "INFO: test $msg"
 
-	TEST_COUNT=10000
+	PORT=10000
 	local extra_args="-o TRANSPARENT"
 	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
 		    ${connect_addr} ${local_addr} "${extra_args}"
-- 
2.40.1

Re: [PATCH mptcp-next v7 2/8] selftests: mptcp: connect: add dedicated port counter

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
> increase port numbers in mptcp_connect.sh.
> 
> This can avoid outputting discontinuous test counters.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 0ca2960c9099..4f40221a86cb 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -33,6 +33,7 @@ do_tcp=0
>  checksum=false
>  filesize=0
>  connect_per_transfer=1
> +PORT=0

Sorry, I forgot to comment on the previous patch: I didn't get why you
cannot use TEST_COUNT for the increment?

So have here:

  PORT=10000

(Or 'PORT_BASE', or even 'port_base' like most "global" variables used
in this file)

>  if [ $tc_loss -eq 100 ];then
>  	tc_loss=1%
> @@ -314,7 +315,7 @@ do_transfer()
>  	local extra_args="$7"
>  
>  	local port
> -	port=$((10000+TEST_COUNT))
> +	port=$((10000+PORT++))

Here:

  port=$((PORT + TEST_COUNT))

>  	TEST_COUNT=$((TEST_COUNT+1))
>  
>  	if [ "$rcvbuf" -gt 0 ]; then
> @@ -710,7 +711,7 @@ EOF
>  
>  	echo "INFO: test $msg"
>  
> -	TEST_COUNT=10000
> +	PORT=10000

And here:

  PORT=20000

So there is only one counter, not 2. Would that not work?

>  	local extra_args="-o TRANSPARENT"
>  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
>  		    ${connect_addr} ${local_addr} "${extra_args}"

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

Re: [PATCH mptcp-next v7 2/8] selftests: mptcp: connect: add dedicated port counter

[Geliang Tang]

Hi Matt,

On Thu, Feb 29, 2024 at 12:23:54PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/02/2024 10:51, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
> > increase port numbers in mptcp_connect.sh.
> > 
> > This can avoid outputting discontinuous test counters.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 0ca2960c9099..4f40221a86cb 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > @@ -33,6 +33,7 @@ do_tcp=0
> >  checksum=false
> >  filesize=0
> >  connect_per_transfer=1
> > +PORT=0
> 
> Sorry, I forgot to comment on the previous patch: I didn't get why you
> cannot use TEST_COUNT for the increment?
> 
> So have here:
> 
>   PORT=10000
> 
> (Or 'PORT_BASE', or even 'port_base' like most "global" variables used
> in this file)
> 
> >  if [ $tc_loss -eq 100 ];then
> >  	tc_loss=1%
> > @@ -314,7 +315,7 @@ do_transfer()
> >  	local extra_args="$7"
> >  
> >  	local port
> > -	port=$((10000+TEST_COUNT))
> > +	port=$((10000+PORT++))
> 
> Here:
> 
>   port=$((PORT + TEST_COUNT))

The first time here TEST_COUNT is 2, so all ports begin with 10002, not
10000. The values of port are changed.

Thanks,
-Geliang

> 
> >  	TEST_COUNT=$((TEST_COUNT+1))
> >  
> >  	if [ "$rcvbuf" -gt 0 ]; then
> > @@ -710,7 +711,7 @@ EOF
> >  
> >  	echo "INFO: test $msg"
> >  
> > -	TEST_COUNT=10000
> > +	PORT=10000
> 
> And here:
> 
>   PORT=20000
> 
> So there is only one counter, not 2. Would that not work?
> 
> >  	local extra_args="-o TRANSPARENT"
> >  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
> >  		    ${connect_addr} ${local_addr} "${extra_args}"
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v7 2/8] selftests: mptcp: connect: add dedicated port counter

[Matthieu Baerts]

On 29/02/2024 13:18, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, Feb 29, 2024 at 12:23:54PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 29/02/2024 10:51, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch adds a new dedicated counter 'PORT' instead of TEST_COUNT to
>>> increase port numbers in mptcp_connect.sh.
>>>
>>> This can avoid outputting discontinuous test counters.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> index 0ca2960c9099..4f40221a86cb 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> @@ -33,6 +33,7 @@ do_tcp=0
>>>  checksum=false
>>>  filesize=0
>>>  connect_per_transfer=1
>>> +PORT=0
>>
>> Sorry, I forgot to comment on the previous patch: I didn't get why you
>> cannot use TEST_COUNT for the increment?
>>
>> So have here:
>>
>>   PORT=10000
>>
>> (Or 'PORT_BASE', or even 'port_base' like most "global" variables used
>> in this file)
>>
>>>  if [ $tc_loss -eq 100 ];then
>>>  	tc_loss=1%
>>> @@ -314,7 +315,7 @@ do_transfer()
>>>  	local extra_args="$7"
>>>  
>>>  	local port
>>> -	port=$((10000+TEST_COUNT))
>>> +	port=$((10000+PORT++))
>>
>> Here:
>>
>>   port=$((PORT + TEST_COUNT))
> 
> The first time here TEST_COUNT is 2, so all ports begin with 10002, not
> 10000. The values of port are changed.

This then?

   port=$((port_base + TEST_COUNT - 2))

Or port_base=9998, but it looks strange.

> 
> Thanks,
> -Geliang
> 
>>
>>>  	TEST_COUNT=$((TEST_COUNT+1))
>>>  
>>>  	if [ "$rcvbuf" -gt 0 ]; then
>>> @@ -710,7 +711,7 @@ EOF
>>>  
>>>  	echo "INFO: test $msg"
>>>  
>>> -	TEST_COUNT=10000
>>> +	PORT=10000
>>
>> And here:
>>
>>   PORT=20000

Here you can also do something like this:

  port_base=$((port_base + 10000 - TEST_COUNT))

Up to you.

>>
>> So there is only one counter, not 2. Would that not work?
>>
>>>  	local extra_args="-o TRANSPARENT"
>>>  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
>>>  		    ${connect_addr} ${local_addr} "${extra_args}"
>>
>> Cheers,
>> Matt
>> -- 
>> Sponsored by the NGI0 Core fund.

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

Re: [PATCH mptcp-next v7 2/8] selftests: mptcp: connect: add dedicated port counter

[Geliang Tang]

On Thu, 2024-02-29 at 13:22 +0100, Matthieu Baerts wrote:
> On 29/02/2024 13:18, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Thu, Feb 29, 2024 at 12:23:54PM +0100, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 29/02/2024 10:51, Geliang Tang wrote:
> > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > 
> > > > This patch adds a new dedicated counter 'PORT' instead of
> > > > TEST_COUNT to
> > > > increase port numbers in mptcp_connect.sh.
> > > > 
> > > > This can avoid outputting discontinuous test counters.
> > > > 
> > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > > ---
> > > >  tools/testing/selftests/net/mptcp/mptcp_connect.sh | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > > > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > > > index 0ca2960c9099..4f40221a86cb 100755
> > > > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > > > @@ -33,6 +33,7 @@ do_tcp=0
> > > >  checksum=false
> > > >  filesize=0
> > > >  connect_per_transfer=1
> > > > +PORT=0
> > > 
> > > Sorry, I forgot to comment on the previous patch: I didn't get
> > > why you
> > > cannot use TEST_COUNT for the increment?
> > > 
> > > So have here:
> > > 
> > >   PORT=10000
> > > 
> > > (Or 'PORT_BASE', or even 'port_base' like most "global" variables
> > > used
> > > in this file)
> > > 
> > > >  if [ $tc_loss -eq 100 ];then
> > > >  	tc_loss=1%
> > > > @@ -314,7 +315,7 @@ do_transfer()
> > > >  	local extra_args="$7"
> > > >  
> > > >  	local port
> > > > -	port=$((10000+TEST_COUNT))
> > > > +	port=$((10000+PORT++))
> > > 
> > > Here:
> > > 
> > >   port=$((PORT + TEST_COUNT))
> > 
> > The first time here TEST_COUNT is 2, so all ports begin with 10002,
> > not
> > 10000. The values of port are changed.
> 
> This then?
> 
>    port=$((port_base + TEST_COUNT - 2))
> 
> Or port_base=9998, but it looks strange.
> 
> > 
> > Thanks,
> > -Geliang
> > 
> > > 
> > > >  	TEST_COUNT=$((TEST_COUNT+1))
> > > >  
> > > >  	if [ "$rcvbuf" -gt 0 ]; then
> > > > @@ -710,7 +711,7 @@ EOF
> > > >  
> > > >  	echo "INFO: test $msg"
> > > >  
> > > > -	TEST_COUNT=10000
> > > > +	PORT=10000
> > > 
> > > And here:
> > > 
> > >   PORT=20000
> 
> Here you can also do something like this:
> 
>   port_base=$((port_base + 10000 - TEST_COUNT))
> 
> Up to you.

Thanks Matt, this works I guess. But becoming more complex. I prefer
the old one.

-Geliang

> 
> > > 
> > > So there is only one counter, not 2. Would that not work?
> > > 
> > > >  	local extra_args="-o TRANSPARENT"
> > > >  	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP
> > > > \
> > > >  		    ${connect_addr} ${local_addr}
> > > > "${extra_args}"
> > > 
> > > Cheers,
> > > Matt
> > > -- 
> > > Sponsored by the NGI0 Core fund.
> 
> Cheers,
> Matt

[PATCH mptcp-next v7 3/8] selftests: mptcp: connect: fix misaligned output

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The first [ OK ] in the output of mptcp_connect.sh misaligns with the
others:

New MPTCP socket can be blocked via sysctl              [ OK ]
INFO: validating network environment with pings
INFO: Using loss of 0.85% delay 16 ms reorder 95% 70% with delay 4ms on
ns1 MPTCP -> ns1 (10.0.1.1:10000      ) MPTCP   (duration   184ms) [ OK ]
ns1 MPTCP -> ns1 (10.0.1.1:10001      ) TCP     (duration    50ms) [ OK ]
ns1 TCP   -> ns1 (10.0.1.1:10002      ) MPTCP   (duration    55ms) [ OK ]

This patch aligns them by using 69 chars to display the first two lines,
and 50 chars for the other. Since 19 chars are used to display duration
time. Also print out a [ OK ] at the end of the 2nd line for consistency.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 4f40221a86cb..77b0b9d5aca0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -274,7 +274,8 @@ check_mptcp_disabled()
 		return 1
 	fi
 
-	echo -e "New MPTCP socket can be blocked via sysctl\t\t[ OK ]"
+	printf "%-69s" "New MPTCP socket can be blocked via sysctl"
+	echo "[ OK ]"
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
 }
@@ -343,7 +344,7 @@ do_transfer()
 	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
 	local result_msg
 	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s" ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port} ${srv_proto})"
-	printf "%s\t" "${result_msg}"
+	printf "%-50s" "${result_msg}"
 
 	if $capture; then
 		local capuser
@@ -836,7 +837,7 @@ check_mptcp_disabled
 
 stop_if_error "The kernel configuration is not valid for MPTCP"
 
-echo "INFO: validating network environment with pings"
+printf "%-69s" "Validating network environment with pings"
 for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
 	do_ping "$ns1" $sender 10.0.1.1
 	do_ping "$ns1" $sender dead:beef:1::1
@@ -858,6 +859,7 @@ done
 mptcp_lib_result_code "${ret}" "ping tests"
 
 stop_if_error "Could not even run ping tests"
+echo "[ OK ]"
 
 [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
 echo -n "INFO: Using loss of $tc_loss "
-- 
2.40.1

[PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The last line in the output of simult_flows.sh misaligns with the other
lines, and all lines are over maximum 75 chars per line.

This patch aligns them by using the newly added print_title() helper,
and truncate them within 75 chars per line to allow it to be displayed
normally in regular terminals without breaking lines.

The new output looks like this:

balanced bwidth                                        7395 max 7906 [ OK ]
balanced bwidth - reverse direction                    7484 max 7906 [ OK ]
balanced bwidth with unbalanced delay                  7394 max 7906 [ OK ]
balanced bwidth with unbalanced delay - reverse direct 7399 max 7906 [ OK ]
unbalanced bwidth                                      7692 max 7906 [ OK ]
unbalanced bwidth - reverse direction                  7614 max 7906 [ OK ]
unbalanced bwidth with unbalanced delay                7425 max 7906 [ OK ]
unbalanced bwidth with unbalanced delay - reverse dire 7473 max 7906 [ OK ]
unbalanced bwidth with opposed, unbalanced delay       7639 max 7906 [ OK ]
unbalanced bwidth with opposed, unbalanced delay - rev 7611 max 7906 [ OK ]

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

diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 467feb17e07b..d5f8521b88d5 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -184,7 +184,7 @@ do_transfer()
 	cmp $cin $sout > /dev/null 2>&1
 	local cmpc=$?
 
-	printf "%-16s" " max $max_time "
+	printf "%-10s" " max $max_time "
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
 	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
 		echo "[ OK ]"
@@ -249,7 +249,7 @@ run_test()
 	fi
 
 	msg+=" - reverse direction"
-	printf "%-60s" "${msg}"
+	printf "%-60s" "$(echo -n ${msg} | cut -c1-54)"
 	do_transfer $large $small $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"
-- 
2.40.1

Re: [PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Geliang Tang]

On Thu, Feb 29, 2024 at 05:51:07PM +0800, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The last line in the output of simult_flows.sh misaligns with the other
> lines, and all lines are over maximum 75 chars per line.
> 
> This patch aligns them by using the newly added print_title() helper,
> and truncate them within 75 chars per line to allow it to be displayed
> normally in regular terminals without breaking lines.

This part of commit log needs to be updated:

    This patch aligns them by reducing the number of spaces before [ OK ] to
    1, and truncate them within 75 chars per line to allow it to be displayed
    normally in regular terminals without breaking lines.

Thanks,
-Geliang

> 
> The new output looks like this:
> 
> balanced bwidth                                        7395 max 7906 [ OK ]
> balanced bwidth - reverse direction                    7484 max 7906 [ OK ]
> balanced bwidth with unbalanced delay                  7394 max 7906 [ OK ]
> balanced bwidth with unbalanced delay - reverse direct 7399 max 7906 [ OK ]
> unbalanced bwidth                                      7692 max 7906 [ OK ]
> unbalanced bwidth - reverse direction                  7614 max 7906 [ OK ]
> unbalanced bwidth with unbalanced delay                7425 max 7906 [ OK ]
> unbalanced bwidth with unbalanced delay - reverse dire 7473 max 7906 [ OK ]
> unbalanced bwidth with opposed, unbalanced delay       7639 max 7906 [ OK ]
> unbalanced bwidth with opposed, unbalanced delay - rev 7611 max 7906 [ OK ]
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/simult_flows.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index 467feb17e07b..d5f8521b88d5 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
> @@ -184,7 +184,7 @@ do_transfer()
>  	cmp $cin $sout > /dev/null 2>&1
>  	local cmpc=$?
>  
> -	printf "%-16s" " max $max_time "
> +	printf "%-10s" " max $max_time "
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
>  	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
>  		echo "[ OK ]"
> @@ -249,7 +249,7 @@ run_test()
>  	fi
>  
>  	msg+=" - reverse direction"
> -	printf "%-60s" "${msg}"
> +	printf "%-60s" "$(echo -n ${msg} | cut -c1-54)"
>  	do_transfer $large $small $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"
> -- 
> 2.40.1
> 

Re: [PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 11:07, Geliang Tang wrote:
> On Thu, Feb 29, 2024 at 05:51:07PM +0800, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> The last line in the output of simult_flows.sh misaligns with the other
>> lines, and all lines are over maximum 75 chars per line.
>>
>> This patch aligns them by using the newly added print_title() helper,
>> and truncate them within 75 chars per line to allow it to be displayed
>> normally in regular terminals without breaking lines.
> 
> This part of commit log needs to be updated:
> 
>     This patch aligns them by reducing the number of spaces before [ OK ] to
>     1, and truncate them within 75 chars per line to allow it to be displayed
>     normally in regular terminals without breaking lines.
> 
> Thanks,
> -Geliang

I don't know if the 2nd part of my previous comment was clear:

> But also, why not having a wider output instead? Why the 75 chars limit?
> 
> I mean: yes, the test names should not be too long. But here, probably
> too late to change, only modify (increase) the value in print_title(), no?
Why having this limit of 75 char? In other words, why not printing a
larger title, e.g. 90 chars or more if needed?

  print_title() {
      printf "%-90s" "${1}"
  }


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

Re: [PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Geliang Tang]

Hi Matt,

On Thu, Feb 29, 2024 at 11:27:29AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/02/2024 11:07, Geliang Tang wrote:
> > On Thu, Feb 29, 2024 at 05:51:07PM +0800, Geliang Tang wrote:
> >> From: Geliang Tang <tanggeliang@kylinos.cn>
> >>
> >> The last line in the output of simult_flows.sh misaligns with the other
> >> lines, and all lines are over maximum 75 chars per line.
> >>
> >> This patch aligns them by using the newly added print_title() helper,
> >> and truncate them within 75 chars per line to allow it to be displayed
> >> normally in regular terminals without breaking lines.
> > 
> > This part of commit log needs to be updated:
> > 
> >     This patch aligns them by reducing the number of spaces before [ OK ] to
> >     1, and truncate them within 75 chars per line to allow it to be displayed
> >     normally in regular terminals without breaking lines.
> > 
> > Thanks,
> > -Geliang
> 
> I don't know if the 2nd part of my previous comment was clear:
> 
> > But also, why not having a wider output instead? Why the 75 chars limit?

"... to allow it to be displayed normally in regular terminals (80 chars)
without breaking lines." 2 chars is reserved for "# " added at the beginning
of each line when running tests in virtme-docker. So I chose 75 chars.

> > 
> > I mean: yes, the test names should not be too long. But here, probably
> > too late to change, only modify (increase) the value in print_title(), no?

I thinks it's not too late, it's the right time to change this, since
printing title with test counter increases 3 chars for each line,
becoming much longer. It makes sense to reduce it this time.

> Why having this limit of 75 char? In other words, why not printing a
> larger title, e.g. 90 chars or more if needed?
> 
>   print_title() {
>       printf "%-90s" "${1}"
>   }

And the lines is too long already, so it makes sense to reduce it, not
increase it, right?

I thinks another option is too keep the length unchanged, not reducing
it, nor increasing it too.

WDYT?

Thanks,
-Geliang

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

Re: [PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 13:01, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, Feb 29, 2024 at 11:27:29AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 29/02/2024 11:07, Geliang Tang wrote:
>>> On Thu, Feb 29, 2024 at 05:51:07PM +0800, Geliang Tang wrote:
>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>
>>>> The last line in the output of simult_flows.sh misaligns with the other
>>>> lines, and all lines are over maximum 75 chars per line.
>>>>
>>>> This patch aligns them by using the newly added print_title() helper,
>>>> and truncate them within 75 chars per line to allow it to be displayed
>>>> normally in regular terminals without breaking lines.
>>>
>>> This part of commit log needs to be updated:
>>>
>>>     This patch aligns them by reducing the number of spaces before [ OK ] to
>>>     1, and truncate them within 75 chars per line to allow it to be displayed
>>>     normally in regular terminals without breaking lines.
>>>
>>> Thanks,
>>> -Geliang
>>
>> I don't know if the 2nd part of my previous comment was clear:
>>
>>> But also, why not having a wider output instead? Why the 75 chars limit?
> 
> "... to allow it to be displayed normally in regular terminals (80 chars)
> without breaking lines." 2 chars is reserved for "# " added at the beginning
> of each line when running tests in virtme-docker. So I chose 75 chars.

Ah OK, I didn't get that "regular terminals are 80 chars wide". To be
honest, I don't know if this is still true. I mean: I never use a
terminal that is only 80 chars wide, at least 100/120. That's why I was
suggesting to show the full info, instead of trunking it.

>>> I mean: yes, the test names should not be too long. But here, probably
>>> too late to change, only modify (increase) the value in print_title(), no?
> 
> I thinks it's not too late, it's the right time to change this, since
> printing title with test counter increases 3 chars for each line,
> becoming much longer. It makes sense to reduce it this time.

But are we not loosing info? e.g. the last one is already:

  "unbalanced bwidth with opposed, unbalanced delay - rev"

If you remove the last 3 chars for the counter, we don't know the
difference with the previous test.

>> Why having this limit of 75 char? In other words, why not printing a
>> larger title, e.g. 90 chars or more if needed?
>>
>>   print_title() {
>>       printf "%-90s" "${1}"
>>   }
> 
> And the lines is too long already, so it makes sense to reduce it, not
> increase it, right?
> 
> I thinks another option is too keep the length unchanged, not reducing
> it, nor increasing it too.

Yes, maybe better, not to "lose" bits. So increasing the title to be
able to align on the last one, and reducing the "${maxtime}" to save a
bit of space?

About the "maxtime", be careful that "selftests: mptcp: simult flows:
re-adapt BW" patch is "blocked" for the moment. So in net and net-next,
we have something like:

  7384 max 7906
  11345 max 11921

(that's 2 more chars than the current version in our tree)

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

Re: [PATCH mptcp-next v7 4/8] selftests: mptcp: simult_flows: fix misaligned output

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 13:18, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/02/2024 13:01, Geliang Tang wrote:
>> Hi Matt,
>>
>> On Thu, Feb 29, 2024 at 11:27:29AM +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 29/02/2024 11:07, Geliang Tang wrote:
>>>> On Thu, Feb 29, 2024 at 05:51:07PM +0800, Geliang Tang wrote:
>>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>>
>>>>> The last line in the output of simult_flows.sh misaligns with the other
>>>>> lines, and all lines are over maximum 75 chars per line.
>>>>>
>>>>> This patch aligns them by using the newly added print_title() helper,
>>>>> and truncate them within 75 chars per line to allow it to be displayed
>>>>> normally in regular terminals without breaking lines.
>>>>
>>>> This part of commit log needs to be updated:
>>>>
>>>>     This patch aligns them by reducing the number of spaces before [ OK ] to
>>>>     1, and truncate them within 75 chars per line to allow it to be displayed
>>>>     normally in regular terminals without breaking lines.
>>>>
>>>> Thanks,
>>>> -Geliang
>>>
>>> I don't know if the 2nd part of my previous comment was clear:
>>>
>>>> But also, why not having a wider output instead? Why the 75 chars limit?
>>
>> "... to allow it to be displayed normally in regular terminals (80 chars)
>> without breaking lines." 2 chars is reserved for "# " added at the beginning
>> of each line when running tests in virtme-docker. So I chose 75 chars.
> 
> Ah OK, I didn't get that "regular terminals are 80 chars wide". To be
> honest, I don't know if this is still true. I mean: I never use a
> terminal that is only 80 chars wide, at least 100/120. That's why I was
> suggesting to show the full info, instead of trunking it.

Out of curiosity: when you launch the tests, do you use a terminal of
that size?

>>>> I mean: yes, the test names should not be too long. But here, probably
>>>> too late to change, only modify (increase) the value in print_title(), no?
>>
>> I thinks it's not too late, it's the right time to change this, since
>> printing title with test counter increases 3 chars for each line,
>> becoming much longer. It makes sense to reduce it this time.
> 
> But are we not loosing info? e.g. the last one is already:
> 
>   "unbalanced bwidth with opposed, unbalanced delay - rev"
> 
> If you remove the last 3 chars for the counter, we don't know the
> difference with the previous test.
> 
>>> Why having this limit of 75 char? In other words, why not printing a
>>> larger title, e.g. 90 chars or more if needed?
>>>
>>>   print_title() {
>>>       printf "%-90s" "${1}"
>>>   }
>>
>> And the lines is too long already, so it makes sense to reduce it, not
>> increase it, right?
>>
>> I thinks another option is too keep the length unchanged, not reducing
>> it, nor increasing it too.
> 
> Yes, maybe better, not to "lose" bits. So increasing the title to be
> able to align on the last one, and reducing the "${maxtime}" to save a
> bit of space?

If it helps to align stuff or to simplify the code, we could also print
the time after the [ OK ] / [FAIL]:

  balanced bwidth                     [ OK ] 7402 max 7906
  balanced bwidth - reverse direction [ OK ] 7402 max 7906
  (...)

Up to you, just an idea.

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

[PATCH mptcp-next v7 5/8] selftests: mptcp: sockopt: unify ipv4/ipv6 as v4/v6

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

ipv4/ipv6 and v4/v6 are mixed in the output of mptcp_sockopt.sh, this patch
unifies them as v4/v6.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 6ed4aa32222f..ae59136b5bb4 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -130,10 +130,10 @@ do_transfer()
 	local local_addr ip
 	if mptcp_lib_is_v6 "${connect_addr}"; then
 		local_addr="::"
-		ip=ipv6
+		ip=v6
 	else
 		local_addr="0.0.0.0"
-		ip=ipv4
+		ip=v4
 	fi
 
 	cmsg="TIMESTAMPNS"
@@ -232,7 +232,7 @@ do_mptcp_sockopt_tests()
 	lret=$?
 
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: SOL_MPTCP getsockopt (ipv6)" 1>&2
+		echo "FAIL: SOL_MPTCP getsockopt (v6)" 1>&2
 		mptcp_lib_result_fail "sockopt v6"
 		ret=$lret
 		return
-- 
2.40.1

Re: [PATCH mptcp-next v7 5/8] selftests: mptcp: sockopt: unify ipv4/ipv6 as v4/v6

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> ipv4/ipv6 and v4/v6 are mixed in the output of mptcp_sockopt.sh, this patch
> unifies them as v4/v6.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 6ed4aa32222f..ae59136b5bb4 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -130,10 +130,10 @@ do_transfer()
>  	local local_addr ip
>  	if mptcp_lib_is_v6 "${connect_addr}"; then
>  		local_addr="::"
> -		ip=ipv6
> +		ip=v6
>  	else
>  		local_addr="0.0.0.0"
> -		ip=ipv4
> +		ip=v4

Mmh, this will change the test name (used in "mptcp_lib_result_*") and
this will break the tracking using the test name [1] while the test is
still doing the same thing.

Maybe this patch is not worth it then, fine to see 'ipv6'. Or only
display a different string in what is printed per test, what you were
doing in patch 5/9 from the v6, but I don't know where this patch is:
did you accidentally drop it?

[1] e.g.
https://netdev.bots.linux.dev/flakes.html?br-cnt=88&tn-needle=mptcp or
on LKFT or our CI

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

Re: [PATCH mptcp-next v7 5/8] selftests: mptcp: sockopt: unify ipv4/ipv6 as v4/v6

[Geliang Tang]

Hi Matt,

On Thu, Feb 29, 2024 at 12:31:21PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/02/2024 10:51, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > ipv4/ipv6 and v4/v6 are mixed in the output of mptcp_sockopt.sh, this patch
> > unifies them as v4/v6.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > index 6ed4aa32222f..ae59136b5bb4 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > @@ -130,10 +130,10 @@ do_transfer()
> >  	local local_addr ip
> >  	if mptcp_lib_is_v6 "${connect_addr}"; then
> >  		local_addr="::"
> > -		ip=ipv6
> > +		ip=v6
> >  	else
> >  		local_addr="0.0.0.0"
> > -		ip=ipv4
> > +		ip=v4
> 
> Mmh, this will change the test name (used in "mptcp_lib_result_*") and
> this will break the tracking using the test name [1] while the test is
> still doing the same thing.
> 
> Maybe this patch is not worth it then, fine to see 'ipv6'. Or only

Do you mean drop this patch? I agree, let's drop it.

> display a different string in what is printed per test, what you were
> doing in patch 5/9 from the v6, but I don't know where this patch is:
> did you accidentally drop it?

5/9 from the v6 is merged into "v7 7/8".

Thanks,
-Geliang

> 
> [1] e.g.
> https://netdev.bots.linux.dev/flakes.html?br-cnt=88&tn-needle=mptcp or
> on LKFT or our CI
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v7 5/8] selftests: mptcp: sockopt: unify ipv4/ipv6 as v4/v6

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 13:08, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, Feb 29, 2024 at 12:31:21PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 29/02/2024 10:51, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> ipv4/ipv6 and v4/v6 are mixed in the output of mptcp_sockopt.sh, this patch
>>> unifies them as v4/v6.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  tools/testing/selftests/net/mptcp/mptcp_sockopt.sh | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
>>> index 6ed4aa32222f..ae59136b5bb4 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
>>> @@ -130,10 +130,10 @@ do_transfer()
>>>  	local local_addr ip
>>>  	if mptcp_lib_is_v6 "${connect_addr}"; then
>>>  		local_addr="::"
>>> -		ip=ipv6
>>> +		ip=v6
>>>  	else
>>>  		local_addr="0.0.0.0"
>>> -		ip=ipv4
>>> +		ip=v4
>>
>> Mmh, this will change the test name (used in "mptcp_lib_result_*") and
>> this will break the tracking using the test name [1] while the test is
>> still doing the same thing.
>>
>> Maybe this patch is not worth it then, fine to see 'ipv6'. Or only
> 
> Do you mean drop this patch? I agree, let's drop it.

Yes, I prefer not to modify the test name printed in the TAP format at
the end. We can modify what is displayed before.

So OK to drop it, easier.

>> display a different string in what is printed per test, what you were
>> doing in patch 5/9 from the v6, but I don't know where this patch is:
>> did you accidentally drop it?
> 
> 5/9 from the v6 is merged into "v7 7/8".

Ah OK, I'm not there yet.

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

[PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
counter, and mptcp_lib_pr_title_counter(), print the test title with
counter. They are used in mptcp_join.sh first.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 1df2d24979a0..7acc6064eb17 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -48,7 +48,6 @@ declare -A all_tests
 declare -a only_tests_ids
 declare -a only_tests_names
 declare -A failed_tests
-TEST_COUNT=0
 TEST_NAME=""
 nr_blank=6
 
@@ -172,7 +171,8 @@ cleanup()
 
 print_title()
 {
-	printf "%03u %s\n" "${TEST_COUNT}" "${TEST_NAME}"
+	MPTCP_LIB_TEST_FORMAT="%03u %s\n"
+	mptcp_lib_pr_title_counter "${TEST_NAME}"
 }
 
 print_check()
@@ -233,7 +233,7 @@ skip_test()
 
 	local i
 	for i in "${only_tests_ids[@]}"; do
-		if [ "${TEST_COUNT}" -eq "${i}" ]; then
+		if [ "${MPTCP_LIB_TEST_COUNTER}" -eq "${i}" ]; then
 			return 1
 		fi
 	done
@@ -268,7 +268,7 @@ reset()
 
 	TEST_NAME="${1}"
 
-	TEST_COUNT=$((TEST_COUNT+1))
+	mptcp_lib_inc_test_counter
 
 	if skip_test; then
 		last_test_ignored=1
@@ -462,7 +462,7 @@ fail_test()
 
 	# just in case a test is marked twice as failed
 	if [ ${last_test_failed} -eq 0 ]; then
-		failed_tests[${TEST_COUNT}]="${TEST_NAME}"
+		failed_tests[${MPTCP_LIB_TEST_COUNTER}]="${TEST_NAME}"
 		dump_stats
 		last_test_failed=1
 	fi
@@ -973,7 +973,7 @@ do_transfer()
 	local srv_proto="$4"
 	local connect_addr="$5"
 
-	local port=$((10000 + TEST_COUNT - 1))
+	local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1))
 	local cappid
 	local FAILING_LINKS=${FAILING_LINKS:-""}
 	local fastclose=${fastclose:-""}
@@ -991,9 +991,9 @@ do_transfer()
 			capuser="-Z $SUDO_USER"
 		fi
 
-		capfile=$(printf "mp_join-%02u-%s.pcap" "$TEST_COUNT" "${listener_ns}")
+		capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}")
 
-		echo "Capturing traffic for test $TEST_COUNT into $capfile"
+		echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile"
 		ip netns exec ${listener_ns} tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
 		cappid=$!
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 763a2989ca6d..cbf0dd2cc4cb 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -411,3 +411,20 @@ mptcp_lib_events() {
 	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
 	pid=$!
 }
+
+MPTCP_LIB_TEST_COUNTER=0
+MPTCP_LIB_TEST_FORMAT=""
+
+mptcp_lib_inc_test_counter() {
+	: "${MPTCP_LIB_TEST_COUNTER:?}"
+
+	MPTCP_LIB_TEST_COUNTER=$((MPTCP_LIB_TEST_COUNTER+1))
+}
+
+#shellcheck disable=SC2059
+mptcp_lib_pr_title_counter() {
+	: "${MPTCP_LIB_TEST_COUNTER:?}"
+	: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
+
+	printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
+}
-- 
2.40.1

Re: [PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
> the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
> Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
> counter, and mptcp_lib_pr_title_counter(), print the test title with
> counter. They are used in mptcp_join.sh first.

Please add the reason, something like: Each MPTCP selftest is having
subtests, and it helps to give them a number to quickly identify them.
This can be managed by mptcp_lib.sh, reusing what has been done here.
The following commit will use these new helpers in the other tests.

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 16 ++++++++--------
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh  | 17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 1df2d24979a0..7acc6064eb17 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -48,7 +48,6 @@ declare -A all_tests
>  declare -a only_tests_ids
>  declare -a only_tests_names
>  declare -A failed_tests
> -TEST_COUNT=0
>  TEST_NAME=""
>  nr_blank=6
>  
> @@ -172,7 +171,8 @@ cleanup()
>  
>  print_title()
>  {
> -	printf "%03u %s\n" "${TEST_COUNT}" "${TEST_NAME}"
> +	MPTCP_LIB_TEST_FORMAT="%03u %s\n"

No need to re-set it all the time, this line can be moved just before
"sourcing" mptcp_lib.sh I think, with a comment like:

  # Here, we have more than 100 tests, having multiple checks
  MPTCP_LIB_TEST_FORMAT="%03u %s\n"

  . "$(dirname "${0}")/mptcp_lib.sh"

> +	mptcp_lib_pr_title_counter "${TEST_NAME}"
>  }
>  
>  print_check()

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 763a2989ca6d..cbf0dd2cc4cb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -411,3 +411,20 @@ mptcp_lib_events() {
>  	ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
>  	pid=$!
>  }
> +
> +MPTCP_LIB_TEST_COUNTER=0
> +MPTCP_LIB_TEST_FORMAT=""

Can you declare them at the top, around the other ones?
(MPTCP_LIB_SUBTESTS, etc.)

And I think you should set the default value there too, like what I was
suggesting in patch 07/12 from v5:

  : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
  MPTCP_LIB_TEST_COUNTER=0

So we can change the format by setting MPTCP_LIB_TEST_FORMAT before
"sourcing" mptcp_lib.sh.

No?

*If* we need to alter the format just for one title, we can always do:

  MPTCP_LIB_TEST_FORMAT="..." \
      mptcp_lib_pr_title_counter "<title>"

But best to avoid that.

> +
> +mptcp_lib_inc_test_counter() {
> +	: "${MPTCP_LIB_TEST_COUNTER:?}"

I don't think that's needed, it has been defined in the file, no?

> +
> +	MPTCP_LIB_TEST_COUNTER=$((MPTCP_LIB_TEST_COUNTER+1))
> +}
> +
> +#shellcheck disable=SC2059

Please always justify why you need to ignore a rule.

Also, always try to reduce the scope as much as possible: just for one
line, for one function, for the whole file → here, move it just before
calling 'printf':

  # shellcheck disable=SC2059 # the format is in a variable
  printf "${MPTCP_LIB_TEST_FORMAT}" (...)

(and add a space after '#')

> +mptcp_lib_pr_title_counter() {
> +	: "${MPTCP_LIB_TEST_COUNTER:?}"
> +	: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> +
> +	printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"

I didn't check: can you not print the title and increment the counter
(via mptcp_lib_inc_test_counter) from here? I think that would be a good
practice to do that instead of having to deal with the counter in
different places.

We would only call "mptcp_lib_inc_test_counter()" from other scripts
when a test is skipped.

> +}

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

Re: [PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[Matthieu Baerts]

On 29/02/2024 12:58, Matthieu Baerts wrote:

(...)

> And I think you should set the default value there too, like what I was
> suggesting in patch 07/12 from v5:
> 
>   : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
>   MPTCP_LIB_TEST_COUNTER=0
> 
> So we can change the format by setting MPTCP_LIB_TEST_FORMAT before
> "sourcing" mptcp_lib.sh.

Mmh, we can also simply set it here:

  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
  MPTCP_LIB_TEST_COUNTER=0

And users of the lib can override it after having "sourced"
mptcp_lib.sh, along with other "global" variables set at the beginning
of the file (with a comment justifying why we need to alter the format).

(...)

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

Re: [PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[Matthieu Baerts]

On 29/02/2024 12:58, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/02/2024 10:51, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
>> the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
>> Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
>> counter, and mptcp_lib_pr_title_counter(), print the test title with
>> counter. They are used in mptcp_join.sh first.
> 
> Please add the reason, something like: Each MPTCP selftest is having
> subtests, and it helps to give them a number to quickly identify them.
> This can be managed by mptcp_lib.sh, reusing what has been done here.
> The following commit will use these new helpers in the other tests.
> 
>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>> ---

(...)

>> +mptcp_lib_pr_title_counter() {
>> +	: "${MPTCP_LIB_TEST_COUNTER:?}"
>> +	: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
>> +
>> +	printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
> 
> I didn't check: can you not print the title and increment the counter
> (via mptcp_lib_inc_test_counter) from here? I think that would be a good
> practice to do that instead of having to deal with the counter in
> different places.
> 
> We would only call "mptcp_lib_inc_test_counter()" from other scripts
> when a test is skipped.

OK, I just saw you added mptcp_lib_print_title() in the next patch. Why
did you not use it for mptcp_join.sh?

Can you not call "mptcp_lib_inc_test_counter" in the "if skip_test" from
mptcp_join.sh and use mptcp_lib_print_title()? (if you do that, I guess
we don't need mptcp_lib_pr_title_counter(), no?)

> 
>> +}
> 
> Cheers,
> Matt

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

Re: [PATCH mptcp-next v7 6/8] selftests: mptcp: add test counter helpers

[Geliang Tang]

On Thu, 2024-02-29 at 13:28 +0100, Matthieu Baerts wrote:
> On 29/02/2024 12:58, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 29/02/2024 10:51, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER
> > > for
> > > the test counter and MPTCP_LIB_TEST_FORMAT for the test print
> > > format.
> > > Also add two helpers, mptcp_lib_inc_test_counter(), increase the
> > > test
> > > counter, and mptcp_lib_pr_title_counter(), print the test title
> > > with
> > > counter. They are used in mptcp_join.sh first.
> > 
> > Please add the reason, something like: Each MPTCP selftest is
> > having
> > subtests, and it helps to give them a number to quickly identify
> > them.
> > This can be managed by mptcp_lib.sh, reusing what has been done
> > here.
> > The following commit will use these new helpers in the other tests.
> > 
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> 
> (...)
> 
> > > +mptcp_lib_pr_title_counter() {
> > > +	: "${MPTCP_LIB_TEST_COUNTER:?}"
> > > +	: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> > > +
> > > +	printf "${MPTCP_LIB_TEST_FORMAT}"
> > > "${MPTCP_LIB_TEST_COUNTER}" "${*}"
> > 
> > I didn't check: can you not print the title and increment the
> > counter
> > (via mptcp_lib_inc_test_counter) from here? I think that would be a
> > good
> > practice to do that instead of having to deal with the counter in
> > different places.
> > 
> > We would only call "mptcp_lib_inc_test_counter()" from other
> > scripts
> > when a test is skipped.
> 
> OK, I just saw you added mptcp_lib_print_title() in the next patch.
> Why
> did you not use it for mptcp_join.sh?
> 
> Can you not call "mptcp_lib_inc_test_counter" in the "if skip_test"
> from
> mptcp_join.sh and use mptcp_lib_print_title()? (if you do that, I
> guess
> we don't need mptcp_lib_pr_title_counter(), no?)

Yes, I dropped mptcp_lib_pr_title_counter in v9.

> 
> > 
> > > +}
> > 
> > Cheers,
> > Matt
> 
> Cheers,
> Matt

[PATCH mptcp-next v7 7/8] selftests: mptcp: print test results with counters

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper mptcp_lib_print_title(), a wrapper of
mptcp_lib_inc_test_counter() and mptcp_lib_pr_title_counter(), to
print out test counter in each test result and increase the counter.
Use this helper to print out test counters for every tests in diag.sh,
mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
and userspace_pm.sh.

diag.sh:

01 no msk on netns creation                          [  ok  ]
02 listen match for dport 10000                      [  ok  ]
03 listen match for sport 10000                      [  ok  ]
04 listen match for saddr and sport                  [  ok  ]
05 all listen sockets                                [  ok  ]

mptcp_connect.sh:

01 New MPTCP socket can be blocked via sysctl                       [ OK ]
02 Validating network environment with pings                        [ OK ]
INFO: Using loss of 0.85% delay 31 ms reorder .. with delay 7ms on ns3eth4
03 ns1 MPTCP -> ns1 (10.0.1.1:10000  ) MPTCP     (duration    69ms) [ OK ]
04 ns1 MPTCP -> ns1 (10.0.1.1:10001  ) TCP       (duration    20ms) [ OK ]
05 ns1 TCP   -> ns1 (10.0.1.1:10002  ) MPTCP     (duration    16ms) [ OK ]

mptcp_sockopt.sh:

01 Transfer v4                                       [ OK ]
02 Mark v4                                           [ OK ]
03 Transfer v6                                       [ OK ]
04 Mark v6                                           [ OK ]
05 SOL_MPTCP sockopt v4                              [ OK ]

pm_netlink.sh:

01 defaults addr list                                [ OK ]
02 simple add/get addr                               [ OK ]
03 dump addrs                                        [ OK ]
04 simple del addr                                   [ OK ]
05 dump addrs after del                              [ OK ]

simult_flows.sh:

01 balanced bwidth                                     7391 max 8456 [ OK ]
02 balanced bwidth - reverse direction                 7403 max 8456 [ OK ]
03 balanced bwidth with unbalanced delay               7429 max 8456 [ OK ]
04 balanced bwidth with unbalanced delay - reverse dir 7485 max 8456 [ OK ]
05 unbalanced bwidth                                   7549 max 8456 [ OK ]

userspace_pm.sh:

01 Created network namespaces ns1, ns2                               [ OK ]
INFO: Make connections
02 Established IPv4 MPTCP Connection ns2 => ns1                      [ OK ]
03 Established IPv6 MPTCP Connection ns2 => ns1                      [ OK ]
INFO: Announce tests
04 ADD_ADDR 10.0.2.2 (ns2) => ns1, invalid token                     [ OK ]
05 ADD_ADDR id:67 10.0.2.2 (ns2) => ns1, reuse port                  [ OK ]

Having test counters helps to quickly identify issues when looking at a
long list of output logs and results.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh     |  7 ++-----
 .../selftests/net/mptcp/mptcp_connect.sh      | 10 +++++-----
 .../testing/selftests/net/mptcp/mptcp_lib.sh  |  5 +++++
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 20 ++++++++++---------
 .../testing/selftests/net/mptcp/pm_netlink.sh |  5 +++--
 .../selftests/net/mptcp/simult_flows.sh       | 14 ++++++++-----
 .../selftests/net/mptcp/userspace_pm.sh       |  3 ++-
 7 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index ff02b699840d..74b19b89d6e6 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -9,7 +9,6 @@
 . "$(dirname "${0}")/mptcp_lib.sh"
 
 ns=""
-test_cnt=1
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
 ret=0
@@ -55,7 +54,7 @@ __chk_nr()
 
 	nr=$(eval $command)
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_title "$msg"
 	if [ "$nr" != "$expected" ]; then
 		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
 			echo "[ skip ] Feature probably not supported"
@@ -69,7 +68,6 @@ __chk_nr()
 		echo "[  ok  ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
-	test_cnt=$((test_cnt+1))
 }
 
 __chk_msk_nr()
@@ -114,7 +112,7 @@ wait_msk_nr()
 		sleep 1
 	done
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_title "$msg"
 	if [ $i -ge $timeout ]; then
 		echo "[ fail ] timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
@@ -127,7 +125,6 @@ wait_msk_nr()
 		echo "[  ok  ]"
 		mptcp_lib_result_pass "${msg}"
 	fi
-	test_cnt=$((test_cnt+1))
 }
 
 chk_msk_fallback_nr()
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 77b0b9d5aca0..096ff8941c5b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -131,7 +131,6 @@ ns2=""
 ns3=""
 ns4=""
 
-TEST_COUNT=0
 TEST_GROUP=""
 
 # This function is used in the cleanup trap
@@ -253,6 +252,7 @@ check_mptcp_disabled()
 	local disabled_ns
 	mptcp_lib_ns_init disabled_ns
 
+	mptcp_lib_print_title "New MPTCP socket can be blocked via sysctl"
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
 		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[ FAIL ]"
@@ -274,7 +274,6 @@ check_mptcp_disabled()
 		return 1
 	fi
 
-	printf "%-69s" "New MPTCP socket can be blocked via sysctl"
 	echo "[ OK ]"
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
@@ -317,7 +316,6 @@ do_transfer()
 
 	local port
 	port=$((10000+PORT++))
-	TEST_COUNT=$((TEST_COUNT+1))
 
 	if [ "$rcvbuf" -gt 0 ]; then
 		extra_args="$extra_args -R $rcvbuf"
@@ -344,7 +342,7 @@ do_transfer()
 	addr_port=$(printf "%s:%d" ${connect_addr} ${port})
 	local result_msg
 	result_msg="$(printf "%.3s %-5s -> %.3s (%-20s) %-5s" ${connector_ns} ${cl_proto} ${listener_ns} ${addr_port} ${srv_proto})"
-	printf "%-50s" "${result_msg}"
+	mptcp_lib_print_title "${result_msg}"
 
 	if $capture; then
 		local capuser
@@ -830,6 +828,7 @@ stop_if_error()
 	fi
 }
 
+MPTCP_LIB_TEST_FORMAT="%02u %-69s"
 make_file "$cin" "client"
 make_file "$sin" "server"
 
@@ -837,7 +836,7 @@ check_mptcp_disabled
 
 stop_if_error "The kernel configuration is not valid for MPTCP"
 
-printf "%-69s" "Validating network environment with pings"
+mptcp_lib_print_title "Validating network environment with pings"
 for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
 	do_ping "$ns1" $sender 10.0.1.1
 	do_ping "$ns1" $sender dead:beef:1::1
@@ -860,6 +859,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
 
 stop_if_error "Could not even run ping tests"
 echo "[ OK ]"
+MPTCP_LIB_TEST_FORMAT="%02u %-50s"
 
 [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
 echo -n "INFO: Using loss of $tc_loss "
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index cbf0dd2cc4cb..358d5b77fc0f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -428,3 +428,8 @@ mptcp_lib_pr_title_counter() {
 
 	printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
 }
+
+mptcp_lib_print_title() {
+	mptcp_lib_inc_test_counter
+	mptcp_lib_pr_title_counter "${*}"
+}
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index ae59136b5bb4..a797e13d3fe7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -161,6 +161,7 @@ do_transfer()
 	wait $spid
 	local rets=$?
 
+	mptcp_lib_print_title "Transfer ${ip}"
 	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
 		echo " client exit code $retc, server $rets" 1>&2
 		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
@@ -174,7 +175,9 @@ do_transfer()
 		ret=1
 		return 1
 	fi
+	echo "[ OK ]"
 
+	mptcp_lib_print_title "Mark ${ip}"
 	if [ $local_addr = "::" ];then
 		check_mark $listener_ns 6 || retc=1
 		check_mark $connector_ns 6 || retc=1
@@ -190,8 +193,10 @@ do_transfer()
 	mptcp_lib_result_code "${rets}" "transfer ${ip}"
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
+		echo "[ OK ]"
 		return 0
 	fi
+	echo "FAIL: Mark ${ip}"
 
 	return 1
 }
@@ -220,23 +225,27 @@ do_mptcp_sockopt_tests()
 	ip netns exec "$ns_sbox" ./mptcp_sockopt
 	lret=$?
 
+	mptcp_lib_print_title "SOL_MPTCP sockopt v4"
 	if [ $lret -ne 0 ]; then
 		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
 		mptcp_lib_result_fail "sockopt v4"
 		ret=$lret
 		return
 	fi
+	echo "[ OK ]"
 	mptcp_lib_result_pass "sockopt v4"
 
 	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
 	lret=$?
 
+	mptcp_lib_print_title "SOL_MPTCP sockopt v6"
 	if [ $lret -ne 0 ]; then
 		echo "FAIL: SOL_MPTCP getsockopt (v6)" 1>&2
 		mptcp_lib_result_fail "sockopt v6"
 		ret=$lret
 		return
 	fi
+	echo "[ OK ]"
 	mptcp_lib_result_pass "sockopt v6"
 }
 
@@ -259,6 +268,7 @@ run_tests()
 
 do_tcpinq_test()
 {
+	mptcp_lib_print_title "TCP_INQ cmsg/ioctl $*"
 	ip netns exec "$ns_sbox" ./mptcp_inq "$@"
 	local lret=$?
 	if [ $lret -ne 0 ];then
@@ -268,7 +278,7 @@ do_tcpinq_test()
 		return $lret
 	fi
 
-	echo "PASS: TCP_INQ cmsg/ioctl $*"
+	echo "[ OK ]"
 	mptcp_lib_result_pass "TCP_INQ: $*"
 	return $lret
 }
@@ -314,15 +324,7 @@ trap cleanup EXIT
 run_tests $ns1 $ns2 10.0.1.1
 run_tests $ns1 $ns2 dead:beef:1::1
 
-if [ $ret -eq 0 ];then
-	echo "PASS: all packets had packet mark set"
-fi
-
 do_mptcp_sockopt_tests
-if [ $ret -eq 0 ];then
-	echo "PASS: SOL_MPTCP getsockopt has expected information"
-fi
-
 do_tcpinq_tests
 
 mptcp_lib_result_print_all_tap
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 427fc5c70b3c..5b9bc25dfef4 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -53,7 +53,7 @@ check()
 	local msg="$3"
 	local rc=0
 
-	printf "%-50s" "$msg"
+	mptcp_lib_print_title "$msg"
 	mptcp_lib_check_output "${err}" "${cmd}" "${expected}" || rc=${?}
 	if [ ${rc} -eq 2 ]; then
 		mptcp_lib_result_fail "${msg} # error ${rc}"
@@ -189,7 +189,8 @@ subflow,backup,fullmesh 10.0.1.1" "          (backup,fullmesh)"
 else
 	for st in fullmesh nofullmesh backup,fullmesh; do
 		st="          (${st})"
-		printf "%-50s%s\n" "${st}" "[SKIP]"
+		mptcp_lib_print_title "${st}"
+		echo "[SKIP]"
 		mptcp_lib_result_skip "${st}"
 	done
 fi
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index d5f8521b88d5..212782301c6f 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -14,7 +14,6 @@ ns3=""
 capture=false
 timeout_poll=30
 timeout_test=$((timeout_poll * 2 + 1))
-test_cnt=1
 ret=0
 bail=0
 slack=50
@@ -126,8 +125,7 @@ do_transfer()
 	local sin=$2
 	local max_time=$3
 	local port
-	port=$((10000+test_cnt))
-	test_cnt=$((test_cnt+1))
+	port=$((10000+MPTCP_LIB_TEST_COUNTER))
 
 	:> "$cout"
 	:> "$sout"
@@ -205,6 +203,12 @@ do_transfer()
 	return 1
 }
 
+print_title()
+{
+	MPTCP_LIB_TEST_FORMAT="%02u %-55s"
+	mptcp_lib_print_title "${*}"
+}
+
 run_test()
 {
 	local rate1=$1
@@ -239,7 +243,7 @@ run_test()
 	# completion (see mptcp_connect): 200ms on each side, add some slack
 	time=$((time + 400 + slack))
 
-	printf "%-60s" "$msg"
+	print_title "$msg"
 	do_transfer $small $large $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"
@@ -249,7 +253,7 @@ run_test()
 	fi
 
 	msg+=" - reverse direction"
-	printf "%-60s" "$(echo -n ${msg} | cut -c1-54)"
+	print_title "$(echo -n ${msg} | cut -c1-54)"
 	do_transfer $large $small $time
 	lret=$?
 	mptcp_lib_result_code "${lret}" "${msg}"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index b0cce8f065d8..62e059e3fb24 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -69,7 +69,8 @@ print_test()
 {
 	test_name="${1}"
 
-	_printf "%-68s" "${test_name}"
+	MPTCP_LIB_TEST_FORMAT="%02u %-68s"
+	mptcp_lib_print_title "${test_name}"
 }
 
 print_results()
-- 
2.40.1

Re: [PATCH mptcp-next v7 7/8] selftests: mptcp: print test results with counters

[Matthieu Baerts]

Hi Geliang,

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper mptcp_lib_print_title(), a wrapper of
> mptcp_lib_inc_test_counter() and mptcp_lib_pr_title_counter(), to
> print out test counter in each test result and increase the counter.
> Use this helper to print out test counters for every tests in diag.sh,
> mptcp_connect.sh, mptcp_sockopt.sh, pm_netlink.sh, simult_flows.sh,
> and userspace_pm.sh.

(...)

> Having test counters helps to quickly identify issues when looking at a
> long list of output logs and results.

Nice!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 77b0b9d5aca0..096ff8941c5b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> @@ -131,7 +131,6 @@ ns2=""
>  ns3=""
>  ns4=""
>  
> -TEST_COUNT=0
>  TEST_GROUP=""
>  
>  # This function is used in the cleanup trap
> @@ -253,6 +252,7 @@ check_mptcp_disabled()
>  	local disabled_ns
>  	mptcp_lib_ns_init disabled_ns
>  
> +	mptcp_lib_print_title "New MPTCP socket can be blocked via sysctl"

Because you have 2 exceptions (larger titles), maybe best to use a helper:

  print_larger_title() {
      # here we don't have the time, a bit longer for the alignment
      MPTCP_LIB_TEST_FORMAT="%02u %-69s" \
          mptcp_lib_print_title "${@}"
  }

>  	# net.mptcp.enabled should be enabled by default
>  	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
>  		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[ FAIL ]"

(...)

> @@ -830,6 +828,7 @@ stop_if_error()
>  	fi
>  }
>  
> +MPTCP_LIB_TEST_FORMAT="%02u %-69s"

... so you don't have to set it here ...

>  make_file "$cin" "client"
>  make_file "$sin" "server"
>  
> @@ -837,7 +836,7 @@ check_mptcp_disabled
>  
>  stop_if_error "The kernel configuration is not valid for MPTCP"
>  
> -printf "%-69s" "Validating network environment with pings"
> +mptcp_lib_print_title "Validating network environment with pings"

(print_larger_title → clearer)

>  for sender in "$ns1" "$ns2" "$ns3" "$ns4";do
>  	do_ping "$ns1" $sender 10.0.1.1
>  	do_ping "$ns1" $sender dead:beef:1::1
> @@ -860,6 +859,7 @@ mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
>  echo "[ OK ]"
> +MPTCP_LIB_TEST_FORMAT="%02u %-50s"

... and here, just to handle 2 exceptions, and it is not clear which
format will be used.

>  
>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
>  echo -n "INFO: Using loss of $tc_loss "

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index ae59136b5bb4..a797e13d3fe7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -161,6 +161,7 @@ do_transfer()
>  	wait $spid
>  	local rets=$?
>  
> +	mptcp_lib_print_title "Transfer ${ip}"

Please add a note in the commit message explaining that
'mptcp_sockopt.sh' now display more detailed results + why (what you had
in a former patch from v6, merged here).

(Note: to replace patch 5/8, you can also use '${ip:2}' here, maybe
prettier? A detail...)

>  	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
>  		echo " client exit code $retc, server $rets" 1>&2
>  		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
> @@ -174,7 +175,9 @@ do_transfer()
>  		ret=1
>  		return 1
>  	fi
> +	echo "[ OK ]"
>  
> +	mptcp_lib_print_title "Mark ${ip}"

(same here for '${ip:2}' if you want to)

>  	if [ $local_addr = "::" ];then
>  		check_mark $listener_ns 6 || retc=1
>  		check_mark $connector_ns 6 || retc=1
> @@ -190,8 +193,10 @@ do_transfer()
>  	mptcp_lib_result_code "${rets}" "transfer ${ip}"
>  
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
> +		echo "[ OK ]"
>  		return 0
>  	fi
> +	echo "FAIL: Mark ${ip}"

I don't think you need to repeat 'Mark ${ip}', it is already in the
title, no?

>  
>  	return 1
>  }

(...)

> @@ -314,15 +324,7 @@ trap cleanup EXIT
>  run_tests $ns1 $ns2 10.0.1.1
>  run_tests $ns1 $ns2 dead:beef:1::1
>  
> -if [ $ret -eq 0 ];then
> -	echo "PASS: all packets had packet mark set"
> -fi
> -
>  do_mptcp_sockopt_tests
> -if [ $ret -eq 0 ];then
> -	echo "PASS: SOL_MPTCP getsockopt has expected information"
> -fi

Please also mention in the commit message that it no longer displays
'PASS:', because it is duplicated info now that the detailed are displayed.

> -
>  do_tcpinq_tests
>  
>  mptcp_lib_result_print_all_tap

(...)

> diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
> index d5f8521b88d5..212782301c6f 100755
> --- a/tools/testing/selftests/net/mptcp/simult_flows.sh
> +++ b/tools/testing/selftests/net/mptcp/simult_flows.sh

(...)

> @@ -205,6 +203,12 @@ do_transfer()
>  	return 1
>  }
>  
> +print_title()
> +{
> +	MPTCP_LIB_TEST_FORMAT="%02u %-55s"

Same here, can be set once above, at the beginning of the file (with a
small comment explaining why we need a bit more space: because we have
more to display :) )

> +	mptcp_lib_print_title "${*}"
> +}
> +
>  run_test()
>  {
>  	local rate1=$1
> @@ -239,7 +243,7 @@ run_test()
>  	# completion (see mptcp_connect): 200ms on each side, add some slack
>  	time=$((time + 400 + slack))
>  
> -	printf "%-60s" "$msg"
> +	print_title "$msg"
>  	do_transfer $small $large $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"
> @@ -249,7 +253,7 @@ run_test()
>  	fi
>  
>  	msg+=" - reverse direction"
> -	printf "%-60s" "$(echo -n ${msg} | cut -c1-54)"
> +	print_title "$(echo -n ${msg} | cut -c1-54)"

As discussed on a previous patch, I think it is best not to cut the
title. (and if we had to, best to do that in the helper, not here)

>  	do_transfer $large $small $time
>  	lret=$?
>  	mptcp_lib_result_code "${lret}" "${msg}"

(...)

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

[PATCH mptcp-next v7 8/8] selftests: mptcp: print test results with colors

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To unify the output formats of all test scripts, this patch adds
four more helpers:

	mptcp_lib_pr_ok()
	mptcp_lib_pr_skip()
	mptcp_lib_pr_fail()
	mptcp_lib_pr_info()

to print out [ OK ], [SKIP], [FAIL] and 'INFO: ' with colors. Use them
in all scripts to print the "ok/skip/fail/info' using the same 'format'.

Having colors helps to quickly identify issues when looking at a long
list of output logs and results.

Note that the mptcp_join.sh tests will now print these keywords with
capital letters, like most of the other tests.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh     | 12 ++--
 .../selftests/net/mptcp/mptcp_connect.sh      | 61 +++++++++----------
 .../testing/selftests/net/mptcp/mptcp_join.sh |  6 +-
 .../testing/selftests/net/mptcp/mptcp_lib.sh  | 35 ++++++++---
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 25 ++++----
 .../testing/selftests/net/mptcp/pm_netlink.sh |  2 +-
 .../selftests/net/mptcp/simult_flows.sh       |  4 +-
 .../selftests/net/mptcp/userspace_pm.sh       | 18 ++----
 8 files changed, 85 insertions(+), 78 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 74b19b89d6e6..f7a729dfc555 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -57,15 +57,15 @@ __chk_nr()
 	mptcp_lib_print_title "$msg"
 	if [ "$nr" != "$expected" ]; then
 		if [ "$nr" = "$skip" ] && ! mptcp_lib_expect_all_features; then
-			echo "[ skip ] Feature probably not supported"
+			mptcp_lib_pr_skip "Feature probably not supported"
 			mptcp_lib_result_skip "${msg}"
 		else
-			echo "[ fail ] expected $expected found $nr"
+			mptcp_lib_pr_fail "expected $expected found $nr"
 			mptcp_lib_result_fail "${msg}"
 			ret=${KSFT_FAIL}
 		fi
 	else
-		echo "[  ok  ]"
+		mptcp_lib_pr_ok
 		mptcp_lib_result_pass "${msg}"
 	fi
 }
@@ -114,15 +114,15 @@ wait_msk_nr()
 
 	mptcp_lib_print_title "$msg"
 	if [ $i -ge $timeout ]; then
-		echo "[ fail ] timeout while expecting $expected max $max last $nr"
+		mptcp_lib_pr_fail "timeout while expecting $expected max $max last $nr"
 		mptcp_lib_result_fail "${msg} # timeout"
 		ret=${KSFT_FAIL}
 	elif [ $nr != $expected ]; then
-		echo "[ fail ] expected $expected found $nr"
+		mptcp_lib_pr_fail "expected $expected found $nr"
 		mptcp_lib_result_fail "${msg} # unexpected result"
 		ret=${KSFT_FAIL}
 	else
-		echo "[  ok  ]"
+		mptcp_lib_pr_ok
 		mptcp_lib_result_pass "${msg}"
 	fi
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 096ff8941c5b..70acbb19f568 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -218,7 +218,7 @@ set_ethtool_flags() {
 	local flags="$3"
 
 	if ip netns exec $ns ethtool -K $dev $flags 2>/dev/null; then
-		echo "INFO: set $ns dev $dev: ethtool -K $flags"
+		mptcp_lib_pr_info "set $ns dev $dev: ethtool -K $flags"
 	fi
 }
 
@@ -255,7 +255,7 @@ check_mptcp_disabled()
 	mptcp_lib_print_title "New MPTCP socket can be blocked via sysctl"
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
-		echo -e "net.mptcp.enabled sysctl is not 1 by default\t\t[ FAIL ]"
+		mptcp_lib_pr_fail "net.mptcp.enabled sysctl is not 1 by default"
 		mptcp_lib_result_fail "net.mptcp.enabled sysctl is not 1 by default"
 		ret=1
 		return 1
@@ -268,13 +268,13 @@ check_mptcp_disabled()
 	mptcp_lib_ns_exit "${disabled_ns}"
 
 	if [ ${err} -eq 0 ]; then
-		echo -e "New MPTCP socket cannot be blocked via sysctl\t\t[ FAIL ]"
+		mptcp_lib_pr_fail "New MPTCP socket cannot be blocked via sysctl"
 		mptcp_lib_result_fail "New MPTCP socket cannot be blocked via sysctl"
 		ret=1
 		return 1
 	fi
 
-	echo "[ OK ]"
+	mptcp_lib_pr_ok
 	mptcp_lib_result_pass "New MPTCP socket can be blocked via sysctl"
 	return 0
 }
@@ -295,7 +295,7 @@ do_ping()
 	ip netns exec ${connector_ns} ping ${ping_args} $connect_addr >/dev/null || rc=1
 
 	if [ $rc -ne 0 ] ; then
-		echo "$listener_ns -> $connect_addr connectivity [ FAIL ]" 1>&2
+		mptcp_lib_pr_fail "$listener_ns -> $connect_addr connectivity"
 		ret=1
 
 		return 1
@@ -330,7 +330,7 @@ do_transfer()
 	fi
 
 	if [ -n "$extra_args" ] && $options_log; then
-		echo "INFO: extra options: $extra_args"
+		mptcp_lib_pr_info "extra options: $extra_args"
 	fi
 	options_log=false
 
@@ -427,7 +427,7 @@ do_transfer()
 	result_msg+=" # time=${duration}ms"
 	printf "(duration %05sms) " "${duration}"
 	if [ ${rets} -ne 0 ] || [ ${retc} -ne 0 ]; then
-		echo "[ FAIL ] client exit code $retc, server $rets" 1>&2
+		mptcp_lib_pr_fail "client exit code $retc, server $rets"
 		echo -e "\nnetns ${listener_ns} socket stat for ${port}:" 1>&2
 		ip netns exec ${listener_ns} ss -Menita 1>&2 -o "sport = :$port"
 		cat /tmp/${listener_ns}.out
@@ -469,14 +469,14 @@ do_transfer()
 	fi
 
 	if [ ${stat_synrx_now_l} -lt ${expect_synrx} ]; then
-		printf "[ FAIL ] lower MPC SYN rx (%d) than expected (%d)\n" \
-			"${stat_synrx_now_l}" "${expect_synrx}" 1>&2
+		mptcp_lib_pr_fail "lower MPC SYN rx (${stat_synrx_now_l})" \
+				  "than expected (${expect_synrx})"
 		retc=1
 	fi
 	if [ ${stat_ackrx_now_l} -lt ${expect_ackrx} ] && [ ${stat_ooo_now} -eq 0 ]; then
 		if [ ${stat_ooo_now} -eq 0 ]; then
-			printf "[ FAIL ] lower MPC ACK rx (%d) than expected (%d)\n" \
-				"${stat_ackrx_now_l}" "${expect_ackrx}" 1>&2
+			mptcp_lib_pr_fail "lower MPC ACK rx (${stat_ackrx_now_l})" \
+					  "than expected (${expect_ackrx})"
 			rets=1
 		else
 			printf "[ Note ] fallback due to TCP OoO"
@@ -491,19 +491,19 @@ do_transfer()
 
 		local csum_err_s_nr=$((csum_err_s - stat_csum_err_s))
 		if [ $csum_err_s_nr -gt 0 ]; then
-			printf "[ FAIL ]\nserver got %d data checksum error[s]" ${csum_err_s_nr}
+			mptcp_lib_pr_fail "server got ${csum_err_s_nr} data checksum error[s]"
 			rets=1
 		fi
 
 		local csum_err_c_nr=$((csum_err_c - stat_csum_err_c))
 		if [ $csum_err_c_nr -gt 0 ]; then
-			printf "[ FAIL ]\nclient got %d data checksum error[s]" ${csum_err_c_nr}
+			mptcp_lib_pr_fail "client got ${csum_err_c_nr} data checksum error[s]"
 			retc=1
 		fi
 	fi
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
-		printf "[ OK ]"
+		mptcp_lib_pr_ok
 		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
 	else
 		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
@@ -534,7 +534,6 @@ do_transfer()
 			"${expect_ackrx}" "${stat_ackrx_now_l}"
 	fi
 
-	echo
 	cat "$capout"
 	[ $retc -eq 0 ] && [ $rets -eq 0 ]
 }
@@ -660,7 +659,7 @@ run_test_transparent()
 	# following function has been exported (T). Not great but better than
 	# checking for a specific kernel version.
 	if ! mptcp_lib_kallsyms_has "T __ip_sock_set_tos$"; then
-		echo "INFO: ${msg} not supported by the kernel: SKIP"
+		mptcp_lib_pr_skip "INFO: ${msg} not supported by the kernel"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
 	fi
@@ -677,7 +676,7 @@ table inet mangle {
 }
 EOF
 	then
-		echo "SKIP: $msg, could not load nft ruleset"
+		mptcp_lib_pr_skip "$msg, could not load nft ruleset"
 		mptcp_lib_fail_if_expected_feature "nft rules"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
@@ -693,7 +692,7 @@ EOF
 
 	if ! ip -net "$listener_ns" $r6flag rule add fwmark 1 lookup 100; then
 		ip netns exec "$listener_ns" nft flush ruleset
-		echo "SKIP: $msg, ip $r6flag rule failed"
+		mptcp_lib_pr_skip "$msg, ip $r6flag rule failed"
 		mptcp_lib_fail_if_expected_feature "ip rule"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
@@ -702,13 +701,13 @@ EOF
 	if ! ip -net "$listener_ns" route add local $local_addr/0 dev lo table 100; then
 		ip netns exec "$listener_ns" nft flush ruleset
 		ip -net "$listener_ns" $r6flag rule del fwmark 1 lookup 100
-		echo "SKIP: $msg, ip route add local $local_addr failed"
+		mptcp_lib_pr_skip "$msg, ip route add local $local_addr failed"
 		mptcp_lib_fail_if_expected_feature "ip route"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
 	fi
 
-	echo "INFO: test $msg"
+	mptcp_lib_pr_info "test $msg"
 
 	PORT=10000
 	local extra_args="-o TRANSPARENT"
@@ -721,12 +720,12 @@ EOF
 	ip -net "$listener_ns" route del local $local_addr/0 dev lo table 100
 
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: $msg, mptcp connection error" 1>&2
+		mptcp_lib_pr_fail "$msg, mptcp connection error"
 		ret=$lret
 		return 1
 	fi
 
-	echo "PASS: $msg"
+	mptcp_lib_pr_info "PASS: $msg"
 	return 0
 }
 
@@ -735,7 +734,7 @@ run_tests_peekmode()
 	local peekmode="$1"
 
 	TEST_GROUP="peek mode: ${peekmode}"
-	echo "INFO: with peek mode: ${peekmode}"
+	mptcp_lib_pr_info "with peek mode: ${peekmode}"
 	run_tests_lo "$ns1" "$ns1" 10.0.1.1 1 "-P ${peekmode}"
 	run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1 "-P ${peekmode}"
 }
@@ -745,12 +744,12 @@ run_tests_mptfo()
 	TEST_GROUP="MPTFO"
 
 	if ! mptcp_lib_kallsyms_has "mptcp_fastopen_"; then
-		echo "INFO: TFO not supported by the kernel: SKIP"
+		mptcp_lib_pr_skip "TFO not supported by the kernel"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
 	fi
 
-	echo "INFO: with MPTFO start"
+	mptcp_lib_pr_info "with MPTFO start"
 	ip netns exec "$ns1" sysctl -q net.ipv4.tcp_fastopen=2
 	ip netns exec "$ns2" sysctl -q net.ipv4.tcp_fastopen=1
 
@@ -762,7 +761,7 @@ run_tests_mptfo()
 
 	ip netns exec "$ns1" sysctl -q net.ipv4.tcp_fastopen=0
 	ip netns exec "$ns2" sysctl -q net.ipv4.tcp_fastopen=0
-	echo "INFO: with MPTFO end"
+	mptcp_lib_pr_info "with MPTFO end"
 }
 
 run_tests_disconnect()
@@ -773,7 +772,7 @@ run_tests_disconnect()
 	TEST_GROUP="full disconnect"
 
 	if ! mptcp_lib_kallsyms_has "mptcp_pm_data_reset$"; then
-		echo "INFO: Full disconnect not supported: SKIP"
+		mptcp_lib_pr_skip "Full disconnect not supported"
 		mptcp_lib_result_skip "${TEST_GROUP}"
 		return
 	fi
@@ -786,7 +785,7 @@ run_tests_disconnect()
 	cin_disconnect="$old_cin"
 	connect_per_transfer=3
 
-	echo "INFO: disconnect"
+	mptcp_lib_pr_info "disconnect"
 	run_tests_lo "$ns1" "$ns1" 10.0.1.1 1 "-I 3 -i $old_cin"
 	run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1 "-I 3 -i $old_cin"
 
@@ -810,7 +809,7 @@ log_if_error()
 	local msg="$1"
 
 	if [ ${ret} -ne 0 ]; then
-		echo "FAIL: ${msg}" 1>&2
+		mptcp_lib_pr_fail "${msg}"
 
 		final_ret=${ret}
 		ret=0
@@ -858,11 +857,11 @@ done
 mptcp_lib_result_code "${ret}" "ping tests"
 
 stop_if_error "Could not even run ping tests"
-echo "[ OK ]"
+mptcp_lib_pr_ok
 MPTCP_LIB_TEST_FORMAT="%02u %-50s"
 
 [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
-echo -n "INFO: Using loss of $tc_loss "
+mptcp_lib_pr_info "Using loss of $tc_loss "
 test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
 
 reorder_delay=$((tc_delay / 4))
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 7acc6064eb17..c3654eb45870 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -190,17 +190,17 @@ print_info()
 
 print_ok()
 {
-	mptcp_lib_print_ok "[ ok ]${1:+ ${*}}"
+	mptcp_lib_pr_ok "${@}"
 }
 
 print_fail()
 {
-	mptcp_lib_print_err "[fail]${1:+ ${*}}"
+	mptcp_lib_pr_fail "${@}"
 }
 
 print_skip()
 {
-	mptcp_lib_print_warn "[skip]${1:+ ${*}}"
+	mptcp_lib_pr_skip "${@}"
 }
 
 # [ $1: fail msg ]
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 358d5b77fc0f..53867f9d212d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -48,6 +48,23 @@ mptcp_lib_print_err() {
 	mptcp_lib_print_color "${MPTCP_LIB_COLOR_RED}${*}"
 }
 
+#shellcheck disable=SC2120
+mptcp_lib_pr_ok() {
+	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
+}
+
+mptcp_lib_pr_skip() {
+	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
+}
+
+mptcp_lib_pr_fail() {
+	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
+}
+
+mptcp_lib_pr_info() {
+	mptcp_lib_print_info "INFO: ${*}"
+}
+
 # SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var can be set when validating all
 # features using the last version of the kernel and the selftests to make sure
 # a test is not being skipped by mistake.
@@ -78,14 +95,14 @@ mptcp_lib_has_file() {
 
 mptcp_lib_check_mptcp() {
 	if ! mptcp_lib_has_file "/proc/sys/net/mptcp/enabled"; then
-		echo "SKIP: MPTCP support is not available"
+		mptcp_lib_pr_skip "MPTCP support is not available"
 		exit ${KSFT_SKIP}
 	fi
 }
 
 mptcp_lib_check_kallsyms() {
 	if ! mptcp_lib_has_file "/proc/kallsyms"; then
-		echo "SKIP: CONFIG_KALLSYMS is missing"
+		mptcp_lib_pr_skip "CONFIG_KALLSYMS is missing"
 		exit ${KSFT_SKIP}
 	fi
 }
@@ -292,7 +309,7 @@ mptcp_lib_check_transfer() {
 	local what="${3}"
 
 	if ! cmp "$in" "$out" > /dev/null 2>&1; then
-		echo "[ FAIL ] $what does not match (in, out):"
+		mptcp_lib_pr_fail "$what does not match (in, out):"
 		mptcp_lib_print_file_err "$in"
 		mptcp_lib_print_file_err "$out"
 
@@ -327,24 +344,24 @@ mptcp_lib_check_tools() {
 		case "${tool}" in
 		"ip")
 			if ! ip -Version &> /dev/null; then
-				mptcp_lib_print_warn "SKIP: Could not run test without ip tool"
+				mptcp_lib_pr_skip "Could not run test without ip tool"
 				exit ${KSFT_SKIP}
 			fi
 			;;
 		"ss")
 			if ! ss -h | grep -q MPTCP; then
-				mptcp_lib_print_warn "SKIP: ss tool does not support MPTCP"
+				mptcp_lib_pr_skip "ss tool does not support MPTCP"
 				exit ${KSFT_SKIP}
 			fi
 			;;
 		"iptables"* | "ip6tables"*)
 			if ! "${tool}" -V &> /dev/null; then
-				mptcp_lib_print_warn "SKIP: Could not run all tests without ${tool}"
+				mptcp_lib_pr_skip "Could not run all tests without ${tool}"
 				exit ${KSFT_SKIP}
 			fi
 			;;
 		*)
-			mptcp_lib_print_err "Internal error: unsupported tool: ${tool}"
+			mptcp_lib_pr_fail "Internal error: unsupported tool: ${tool}"
 			exit ${KSFT_FAIL}
 			;;
 		esac
@@ -363,13 +380,13 @@ mptcp_lib_check_output() {
 	fi
 
 	if [ ${cmd_ret} -ne 0 ]; then
-		mptcp_lib_print_err "[FAIL] command execution '${cmd}' stderr"
+		mptcp_lib_pr_fail "command execution '${cmd}' stderr"
 		cat "${err}"
 		return 2
 	elif [ "${out}" = "${expected}" ]; then
 		return 0
 	else
-		mptcp_lib_print_err "[FAIL] expected '${expected}' got '${out}'"
+		mptcp_lib_pr_fail "expected '${expected}' got '${out}'"
 		return 1
 	fi
 }
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index a797e13d3fe7..d695561fbf46 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -103,7 +103,8 @@ check_mark()
 	local v
 	for v in $values; do
 		if [ $v -ne 0 ]; then
-			echo "FAIL: got $tables $values in ns $ns , not 0 - not all expected packets marked" 1>&2
+			mptcp_lib_pr_fail "got $tables $values in ns $ns," \
+					  "not 0 - not all expected packets marked"
 			ret=1
 			return 1
 		fi
@@ -175,7 +176,7 @@ do_transfer()
 		ret=1
 		return 1
 	fi
-	echo "[ OK ]"
+	mptcp_lib_pr_ok
 
 	mptcp_lib_print_title "Mark ${ip}"
 	if [ $local_addr = "::" ];then
@@ -193,10 +194,10 @@ do_transfer()
 	mptcp_lib_result_code "${rets}" "transfer ${ip}"
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ];then
-		echo "[ OK ]"
+		mptcp_lib_pr_ok
 		return 0
 	fi
-	echo "FAIL: Mark ${ip}"
+	mptcp_lib_pr_fail "Mark ${ip}"
 
 	return 1
 }
@@ -217,7 +218,7 @@ do_mptcp_sockopt_tests()
 	local lret=0
 
 	if ! mptcp_lib_kallsyms_has "mptcp_diag_fill_info$"; then
-		echo "INFO: MPTCP sockopt not supported: SKIP"
+		mptcp_lib_pr_skip "MPTCP sockopt not supported"
 		mptcp_lib_result_skip "sockopt"
 		return
 	fi
@@ -227,12 +228,12 @@ do_mptcp_sockopt_tests()
 
 	mptcp_lib_print_title "SOL_MPTCP sockopt v4"
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
+		mptcp_lib_pr_fail "SOL_MPTCP getsockopt"
 		mptcp_lib_result_fail "sockopt v4"
 		ret=$lret
 		return
 	fi
-	echo "[ OK ]"
+	mptcp_lib_pr_ok
 	mptcp_lib_result_pass "sockopt v4"
 
 	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
@@ -240,12 +241,12 @@ do_mptcp_sockopt_tests()
 
 	mptcp_lib_print_title "SOL_MPTCP sockopt v6"
 	if [ $lret -ne 0 ]; then
-		echo "FAIL: SOL_MPTCP getsockopt (v6)" 1>&2
+		mptcp_lib_pr_fail "SOL_MPTCP getsockopt (v6)"
 		mptcp_lib_result_fail "sockopt v6"
 		ret=$lret
 		return
 	fi
-	echo "[ OK ]"
+	mptcp_lib_pr_ok
 	mptcp_lib_result_pass "sockopt v6"
 }
 
@@ -273,12 +274,12 @@ do_tcpinq_test()
 	local lret=$?
 	if [ $lret -ne 0 ];then
 		ret=$lret
-		echo "FAIL: mptcp_inq $*" 1>&2
+		mptcp_lib_pr_fail "mptcp_inq $*"
 		mptcp_lib_result_fail "TCP_INQ: $*"
 		return $lret
 	fi
 
-	echo "[ OK ]"
+	mptcp_lib_pr_ok
 	mptcp_lib_result_pass "TCP_INQ: $*"
 	return $lret
 }
@@ -288,7 +289,7 @@ do_tcpinq_tests()
 	local lret=0
 
 	if ! mptcp_lib_kallsyms_has "mptcp_ioctl$"; then
-		echo "INFO: TCP_INQ not supported: SKIP"
+		mptcp_lib_pr_skip "INFO: TCP_INQ not supported"
 		mptcp_lib_result_skip "TCP_INQ"
 		return
 	fi
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 5b9bc25dfef4..69ffff8b076b 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -190,7 +190,7 @@ else
 	for st in fullmesh nofullmesh backup,fullmesh; do
 		st="          (${st})"
 		mptcp_lib_print_title "${st}"
-		echo "[SKIP]"
+		mptcp_lib_pr_skip
 		mptcp_lib_result_skip "${st}"
 	done
 fi
diff --git a/tools/testing/selftests/net/mptcp/simult_flows.sh b/tools/testing/selftests/net/mptcp/simult_flows.sh
index 212782301c6f..c5dbe083e47f 100755
--- a/tools/testing/selftests/net/mptcp/simult_flows.sh
+++ b/tools/testing/selftests/net/mptcp/simult_flows.sh
@@ -185,12 +185,12 @@ do_transfer()
 	printf "%-10s" " max $max_time "
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ] && \
 	   [ $cmpc -eq 0 ] && [ $cmps -eq 0 ]; then
-		echo "[ OK ]"
+		mptcp_lib_pr_ok
 		cat "$capout"
 		return 0
 	fi
 
-	echo " [ fail ]"
+	mptcp_lib_pr_fail
 	echo "client exit code $retc, server $rets" 1>&2
 	echo -e "\nnetns ${ns3} socket stat for $port:" 1>&2
 	ip netns exec ${ns3} ss -nita 1>&2 -o "sport = :$port"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 62e059e3fb24..0c5ba35f6af4 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -61,7 +61,7 @@ _printf() {
 
 print_title()
 {
-	_printf "INFO: %s\n" "${1}"
+	mptcp_lib_pr_info "${1}"
 }
 
 # $1: test name
@@ -73,33 +73,23 @@ print_test()
 	mptcp_lib_print_title "${test_name}"
 }
 
-print_results()
-{
-	_printf "[%s]\n" "${1}"
-}
-
 test_pass()
 {
-	print_results " OK "
+	mptcp_lib_pr_ok
 	mptcp_lib_result_pass "${test_name}"
 }
 
 test_skip()
 {
-	print_results "SKIP"
+	mptcp_lib_pr_skip
 	mptcp_lib_result_skip "${test_name}"
 }
 
 # $1: msg
 test_fail()
 {
-	print_results "FAIL"
+	mptcp_lib_pr_fail "${@}"
 	ret=1
-
-	if [ -n "${1}" ]; then
-		_printf "\t%s\n" "${1}"
-	fi
-
 	mptcp_lib_result_fail "${test_name}"
 }
 
-- 
2.40.1

Re: selftests: mptcp: print test results with colors: 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/8094234270

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


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 v7 8/8] selftests: mptcp: print test results with colors

[Matthieu Baerts]

On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To unify the output formats of all test scripts, this patch adds
> four more helpers:
> 
> 	mptcp_lib_pr_ok()
> 	mptcp_lib_pr_skip()
> 	mptcp_lib_pr_fail()
> 	mptcp_lib_pr_info()
> 
> to print out [ OK ], [SKIP], [FAIL] and 'INFO: ' with colors. Use them
> in all scripts to print the "ok/skip/fail/info' using the same 'format'.
> 
> Having colors helps to quickly identify issues when looking at a long
> list of output logs and results.
> 
> Note that the mptcp_join.sh tests will now print these keywords with
> capital letters, like most of the other tests.

I think it is not just mptcp_join.sh that was using different keywords.

  Note that now all print the same keywords, which was not the case
  before, but it is good to uniform that.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> index 096ff8941c5b..70acbb19f568 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh

(...)

> @@ -534,7 +534,6 @@ do_transfer()
>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>  	fi
>  
> -	echo

Just to be sure: do we still have something good in case of error?

I mean: why were we not printing a new line before? Maybe there was no
reason, or maybe sometimes we print more details? (syn cookies?)

Maybe that's fine with the new output, it is just to know if you checked
that.

>  	cat "$capout"
>  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
>  }

(...)

> @@ -810,7 +809,7 @@ log_if_error()
>  	local msg="$1"
>  
>  	if [ ${ret} -ne 0 ]; then
> -		echo "FAIL: ${msg}" 1>&2
> +		mptcp_lib_pr_fail "${msg}"

Good to mention in the commit message that all errors messages are
printed to 'stdout' now, as part of the uniformation. (or do that in an
dedicated commit)

>  
>  		final_ret=${ret}
>  		ret=0
> @@ -858,11 +857,11 @@ done
>  mptcp_lib_result_code "${ret}" "ping tests"
>  
>  stop_if_error "Could not even run ping tests"
> -echo "[ OK ]"
> +mptcp_lib_pr_ok
>  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
>  
>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root netem loss random $tc_loss delay ${tc_delay}ms
> -echo -n "INFO: Using loss of $tc_loss "

Here, it was using 'echo -n' ...

> +mptcp_lib_pr_info "Using loss of $tc_loss "

Now a new line will be printed at the end (with a trailing whitespace)

>  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "

And this will be printed in a new line, it will look strange I think

Maybe use a variable to store all the info?

  tc_info="loss of $tc_loss "
  test "$tc_delay" -gt 0 && tc_info+="delay $tc_delay ms "
  (...)
  mptcp_lib_pr_info "Using ${tc_info}"

>  
>  reorder_delay=$((tc_delay / 4))

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 358d5b77fc0f..53867f9d212d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -48,6 +48,23 @@ mptcp_lib_print_err() {
>  	mptcp_lib_print_color "${MPTCP_LIB_COLOR_RED}${*}"
>  }
>  
> +#shellcheck disable=SC2120

Add a space after '#' and add a comment justifying the exception, like
the one I shared in patch 1/9 from v6:

  # shellcheck disable=SC2120  # parameters are optional

Out of curiosity, we don't need this for 'skip' and 'fail'?

> +mptcp_lib_pr_ok() {
> +	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_skip() {
> +	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_fail() {
> +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
> +}
> +
> +mptcp_lib_pr_info() {
> +	mptcp_lib_print_info "INFO: ${*}"
> +}
> +
>  # SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var can be set when validating all
>  # features using the last version of the kernel and the selftests to make sure
>  # a test is not being skipped by mistake.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index a797e13d3fe7..d695561fbf46 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

(...)

> @@ -217,7 +218,7 @@ do_mptcp_sockopt_tests()
>  	local lret=0
>  
>  	if ! mptcp_lib_kallsyms_has "mptcp_diag_fill_info$"; then
> -		echo "INFO: MPTCP sockopt not supported: SKIP"
> +		mptcp_lib_pr_skip "MPTCP sockopt not supported"
>  		mptcp_lib_result_skip "sockopt"
>  		return
>  	fi
> @@ -227,12 +228,12 @@ do_mptcp_sockopt_tests()
>  
>  	mptcp_lib_print_title "SOL_MPTCP sockopt v4"
>  	if [ $lret -ne 0 ]; then
> -		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
> +		mptcp_lib_pr_fail "SOL_MPTCP getsockopt"

No need to repeat what is already in the title. (I didn't check them
all, but I guess some other subtests here repeat stuff already in the
title, e.g. the same for v6)

>  		mptcp_lib_result_fail "sockopt v4"
>  		ret=$lret
>  		return
>  	fi
> -	echo "[ OK ]"
> +	mptcp_lib_pr_ok
>  	mptcp_lib_result_pass "sockopt v4"
>  
>  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6

(...)

> @@ -288,7 +289,7 @@ do_tcpinq_tests()
>  	local lret=0
>  
>  	if ! mptcp_lib_kallsyms_has "mptcp_ioctl$"; then
> -		echo "INFO: TCP_INQ not supported: SKIP"
> +		mptcp_lib_pr_skip "INFO: TCP_INQ not supported"

You can remove 'INFO: ' I  suppose.

>  		mptcp_lib_result_skip "TCP_INQ"
>  		return
>  	fi

(...)

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

Re: [PATCH mptcp-next v7 8/8] selftests: mptcp: print test results with colors

[Geliang Tang]

On Thu, 2024-02-29 at 14:04 +0100, Matthieu Baerts wrote:
> On 29/02/2024 10:51, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > To unify the output formats of all test scripts, this patch adds
> > four more helpers:
> > 
> > 	mptcp_lib_pr_ok()
> > 	mptcp_lib_pr_skip()
> > 	mptcp_lib_pr_fail()
> > 	mptcp_lib_pr_info()
> > 
> > to print out [ OK ], [SKIP], [FAIL] and 'INFO: ' with colors. Use
> > them
> > in all scripts to print the "ok/skip/fail/info' using the same
> > 'format'.
> > 
> > Having colors helps to quickly identify issues when looking at a
> > long
> > list of output logs and results.
> > 
> > Note that the mptcp_join.sh tests will now print these keywords
> > with
> > capital letters, like most of the other tests.
> 
> I think it is not just mptcp_join.sh that was using different
> keywords.
> 
>   Note that now all print the same keywords, which was not the case
>   before, but it is good to uniform that.
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > index 096ff8941c5b..70acbb19f568 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
> 
> (...)
> 
> > @@ -534,7 +534,6 @@ do_transfer()
> >  			"${expect_ackrx}" "${stat_ackrx_now_l}"
> >  	fi
> >  
> > -	echo
> 
> Just to be sure: do we still have something good in case of error?
> 
> I mean: why were we not printing a new line before? Maybe there was
> no
> reason, or maybe sometimes we print more details? (syn cookies?)
> 
> Maybe that's fine with the new output, it is just to know if you
> checked
> that.

This 'echo' does need to be dropped. Here use mptcp_lib_pr_ok instead
of 'printf "[ OK ]"'. 'printf "[ OK ]"' needs a new line but
mptcp_lib_pr_ok don't need:

 	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
-		printf "[ OK ]"
+		mptcp_lib_pr_ok
 		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
 	else
 		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
@@ -540,7 +540,6 @@  do_transfer()
 			"${expect_ackrx}" "${stat_ackrx_now_l}"
 	fi
 
-	echo
 	cat "$capout"

> 
> >  	cat "$capout"
> >  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
> >  }
> 
> (...)
> 
> > @@ -810,7 +809,7 @@ log_if_error()
> >  	local msg="$1"
> >  
> >  	if [ ${ret} -ne 0 ]; then
> > -		echo "FAIL: ${msg}" 1>&2
> > +		mptcp_lib_pr_fail "${msg}"
> 
> Good to mention in the commit message that all errors messages are
> printed to 'stdout' now, as part of the uniformation. (or do that in
> an
> dedicated commit)

A new commit added in v9.

> 
> >  
> >  		final_ret=${ret}
> >  		ret=0
> > @@ -858,11 +857,11 @@ done
> >  mptcp_lib_result_code "${ret}" "ping tests"
> >  
> >  stop_if_error "Could not even run ping tests"
> > -echo "[ OK ]"
> > +mptcp_lib_pr_ok
> >  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
> >  
> >  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root
> > netem loss random $tc_loss delay ${tc_delay}ms
> > -echo -n "INFO: Using loss of $tc_loss "
> 
> Here, it was using 'echo -n' ...

I keep this 'echo -n' unchanged.

> 
> > +mptcp_lib_pr_info "Using loss of $tc_loss "
> 
> Now a new line will be printed at the end (with a trailing
> whitespace)
> 
> >  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
> 
> And this will be printed in a new line, it will look strange I think
> 
> Maybe use a variable to store all the info?
> 
>   tc_info="loss of $tc_loss "
>   test "$tc_delay" -gt 0 && tc_info+="delay $tc_delay ms "
>   (...)
>   mptcp_lib_pr_info "Using ${tc_info}"
> 
> >  
> >  reorder_delay=$((tc_delay / 4))
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > index 358d5b77fc0f..53867f9d212d 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > @@ -48,6 +48,23 @@ mptcp_lib_print_err() {
> >  	mptcp_lib_print_color "${MPTCP_LIB_COLOR_RED}${*}"
> >  }
> >  
> > +#shellcheck disable=SC2120
> 
> Add a space after '#' and add a comment justifying the exception,
> like
> the one I shared in patch 1/9 from v6:
> 
>   # shellcheck disable=SC2120  # parameters are optional
> 
> Out of curiosity, we don't need this for 'skip' and 'fail'?

Not a single parameter was passed to mptcp_lib_pr_ok at a time.But 'skip' or 'fail' sometimes has parameters passed in.

Thanks,
-Geliang

> 
> > +mptcp_lib_pr_ok() {
> > +	mptcp_lib_print_ok "[ OK ]${1:+ ${*}}"
> > +}
> > +
> > +mptcp_lib_pr_skip() {
> > +	mptcp_lib_print_warn "[SKIP]${1:+ ${*}}"
> > +}
> > +
> > +mptcp_lib_pr_fail() {
> > +	mptcp_lib_print_err "[FAIL]${1:+ ${*}}"
> > +}
> > +
> > +mptcp_lib_pr_info() {
> > +	mptcp_lib_print_info "INFO: ${*}"
> > +}
> > +
> >  # SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var can be set when
> > validating all
> >  # features using the last version of the kernel and the selftests
> > to make sure
> >  # a test is not being skipped by mistake.
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > index a797e13d3fe7..d695561fbf46 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> 
> (...)
> 
> > @@ -217,7 +218,7 @@ do_mptcp_sockopt_tests()
> >  	local lret=0
> >  
> >  	if ! mptcp_lib_kallsyms_has "mptcp_diag_fill_info$"; then
> > -		echo "INFO: MPTCP sockopt not supported: SKIP"
> > +		mptcp_lib_pr_skip "MPTCP sockopt not supported"
> >  		mptcp_lib_result_skip "sockopt"
> >  		return
> >  	fi
> > @@ -227,12 +228,12 @@ do_mptcp_sockopt_tests()
> >  
> >  	mptcp_lib_print_title "SOL_MPTCP sockopt v4"
> >  	if [ $lret -ne 0 ]; then
> > -		echo "FAIL: SOL_MPTCP getsockopt" 1>&2
> > +		mptcp_lib_pr_fail "SOL_MPTCP getsockopt"
> 
> No need to repeat what is already in the title. (I didn't check them
> all, but I guess some other subtests here repeat stuff already in the
> title, e.g. the same for v6)
> 
> >  		mptcp_lib_result_fail "sockopt v4"
> >  		ret=$lret
> >  		return
> >  	fi
> > -	echo "[ OK ]"
> > +	mptcp_lib_pr_ok
> >  	mptcp_lib_result_pass "sockopt v4"
> >  
> >  	ip netns exec "$ns_sbox" ./mptcp_sockopt -6
> 
> (...)
> 
> > @@ -288,7 +289,7 @@ do_tcpinq_tests()
> >  	local lret=0
> >  
> >  	if ! mptcp_lib_kallsyms_has "mptcp_ioctl$"; then
> > -		echo "INFO: TCP_INQ not supported: SKIP"
> > +		mptcp_lib_pr_skip "INFO: TCP_INQ not supported"
> 
> You can remove 'INFO: ' I  suppose.
> 
> >  		mptcp_lib_result_skip "TCP_INQ"
> >  		return
> >  	fi
> 
> (...)
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v7 8/8] selftests: mptcp: print test results with colors

[Matthieu Baerts]

Hi Geliang,

On 01/03/2024 08:16, Geliang Tang wrote:
> On Thu, 2024-02-29 at 14:04 +0100, Matthieu Baerts wrote:
>> On 29/02/2024 10:51, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> To unify the output formats of all test scripts, this patch adds
>>> four more helpers:
>>>
>>> 	mptcp_lib_pr_ok()
>>> 	mptcp_lib_pr_skip()
>>> 	mptcp_lib_pr_fail()
>>> 	mptcp_lib_pr_info()
>>>
>>> to print out [ OK ], [SKIP], [FAIL] and 'INFO: ' with colors. Use
>>> them
>>> in all scripts to print the "ok/skip/fail/info' using the same
>>> 'format'.
>>>
>>> Having colors helps to quickly identify issues when looking at a
>>> long
>>> list of output logs and results.
>>>
>>> Note that the mptcp_join.sh tests will now print these keywords
>>> with
>>> capital letters, like most of the other tests.
>>
>> I think it is not just mptcp_join.sh that was using different
>> keywords.
>>
>>   Note that now all print the same keywords, which was not the case
>>   before, but it is good to uniform that.
>>
>> (...)
>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> index 096ff8941c5b..70acbb19f568 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
>>
>> (...)
>>
>>> @@ -534,7 +534,6 @@ do_transfer()
>>>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>>>  	fi
>>>  
>>> -	echo
>>
>> Just to be sure: do we still have something good in case of error?
>>
>> I mean: why were we not printing a new line before? Maybe there was
>> no
>> reason, or maybe sometimes we print more details? (syn cookies?)
>>
>> Maybe that's fine with the new output, it is just to know if you
>> checked
>> that.
> 
> This 'echo' does need to be dropped. Here use mptcp_lib_pr_ok instead
> of 'printf "[ OK ]"'. 'printf "[ OK ]"' needs a new line but
> mptcp_lib_pr_ok don't need:

Yes, but my point was: I guess there was a reason to have:

  print "[ OK ]"

  (...)

  echo

and not just one line with:

  echo "[ OK ]"


After a quick look, it looks like that's because we want to add info
depending on some conditions, see all the "WARN" messages in between.

I guess we should instead move the "pr_ok" to the end, something like:


  local extra

  (...)

  ### <= the 'printf [ OK ]' was here, now moved to the end ↓

  if (...); then
      extra+=" WARN (...)"
  fi

  (...)

  if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
      mptcp_lib_pr_ok "${extra:1}"
      mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
  else
      if [ -n "${extra}" ]; then
          mptcp_lib_print_warn "${extra:1}"
      fi
      mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
  fi

  cat "$capout"
  [ $retc -eq 0 ] && [ $rets -eq 0 ]


no?

> 
>  	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
> -		printf "[ OK ]"
> +		mptcp_lib_pr_ok
>  		mptcp_lib_result_pass "${TEST_GROUP}: ${result_msg}"
>  	else
>  		mptcp_lib_result_fail "${TEST_GROUP}: ${result_msg}"
> @@ -540,7 +540,6 @@  do_transfer()
>  			"${expect_ackrx}" "${stat_ackrx_now_l}"
>  	fi
>  
> -	echo
>  	cat "$capout"
> 
>>
>>>  	cat "$capout"
>>>  	[ $retc -eq 0 ] && [ $rets -eq 0 ]
>>>  }
>>
>> (...)
>>
>>> @@ -810,7 +809,7 @@ log_if_error()
>>>  	local msg="$1"
>>>  
>>>  	if [ ${ret} -ne 0 ]; then
>>> -		echo "FAIL: ${msg}" 1>&2
>>> +		mptcp_lib_pr_fail "${msg}"
>>
>> Good to mention in the commit message that all errors messages are
>> printed to 'stdout' now, as part of the uniformation. (or do that in
>> an
>> dedicated commit)
> 
> A new commit added in v9.

Thanks!

>>
>>>  
>>>  		final_ret=${ret}
>>>  		ret=0
>>> @@ -858,11 +857,11 @@ done
>>>  mptcp_lib_result_code "${ret}" "ping tests"
>>>  
>>>  stop_if_error "Could not even run ping tests"
>>> -echo "[ OK ]"
>>> +mptcp_lib_pr_ok
>>>  MPTCP_LIB_TEST_FORMAT="%02u %-50s"
>>>  
>>>  [ -n "$tc_loss" ] && tc -net "$ns2" qdisc add dev ns2eth3 root
>>> netem loss random $tc_loss delay ${tc_delay}ms
>>> -echo -n "INFO: Using loss of $tc_loss "
>>
>> Here, it was using 'echo -n' ...
> 
> I keep this 'echo -n' unchanged.

You don't use pr_info with the appended message, as suggested below?

> 
>>
>>> +mptcp_lib_pr_info "Using loss of $tc_loss "
>>
>> Now a new line will be printed at the end (with a trailing
>> whitespace)
>>
>>>  test "$tc_delay" -gt 0 && echo -n "delay $tc_delay ms "
>>
>> And this will be printed in a new line, it will look strange I think
>>
>> Maybe use a variable to store all the info?
>>
>>   tc_info="loss of $tc_loss "
>>   test "$tc_delay" -gt 0 && tc_info+="delay $tc_delay ms "
>>   (...)
>>   mptcp_lib_pr_info "Using ${tc_info}"
>>
>>>  
>>>  reorder_delay=$((tc_delay / 4))
>>
>> (...)
>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> index 358d5b77fc0f..53867f9d212d 100644
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
>>> @@ -48,6 +48,23 @@ mptcp_lib_print_err() {
>>>  	mptcp_lib_print_color "${MPTCP_LIB_COLOR_RED}${*}"
>>>  }
>>>  
>>> +#shellcheck disable=SC2120
>>
>> Add a space after '#' and add a comment justifying the exception,
>> like
>> the one I shared in patch 1/9 from v6:
>>
>>   # shellcheck disable=SC2120  # parameters are optional
>>
>> Out of curiosity, we don't need this for 'skip' and 'fail'?
> 
> Not a single parameter was passed to mptcp_lib_pr_ok at a time.
> But 'skip' or 'fail' sometimes has parameters passed in.

OK, I see. It is just strange to put it only on one, and it would not
hurt to put it on the others as well, but OK, anyway a false positive
from shellcheck.

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