[PATCH mptcp-next 0/3] Squash to "selftests: mptcp: add a test case for MSG_PEEK"

[PATCH mptcp-next 0/3] Squash to "selftests: mptcp: add a test case for MSG_PEEK"

[Matthieu Baerts]

I started this because I prefer avoiding setting a global variable before
calling a function and reset it later in order to take a specific path in the
called function. While at it, I also reduced duplicated code.

But good thing, I also fixed two unwanted behaviours, see patches 1/3 and 2/3.

Matthieu Baerts (3):
  selftests: mptcp: use 2 different 'peek' modes
  selftests: mptcp: same NS for Peek tests in IPv6
  selftests: mptcp: avoid non-const global variables

 .../selftests/net/mptcp/mptcp_connect.sh      | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

-- 
2.30.2

[PATCH mptcp-next 1/3] selftests: mptcp: use 2 different 'peek' modes

[Matthieu Baerts]

When refactoring the code around (see next patches), I noticed the same
'peek mode' was used twice.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 319d1662f4fb..28530f1bd285 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -740,7 +740,7 @@ done
 
 testpeek=true
 options_log=true
-peekmode="saveAfterPeek"
+peekmode="saveWithPeek"
 run_tests_lo "$ns1" "$ns1" 10.0.1.1 1
 run_tests_lo "$ns1" $sender dead:beef:1::1 1
 
-- 
2.30.2

[PATCH mptcp-next 2/3] selftests: mptcp: same NS for Peek tests in IPv6

[Matthieu Baerts]

Also when refactoring the code around (see the next patch), I noticed we
were using different NS for the 'peek mode' tests in IPv6: $sender was
set in a 'for' loop before. Certainly coming from a copy-paste and not
what we want to have.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 28530f1bd285..dbb4aea8b636 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -742,12 +742,12 @@ testpeek=true
 options_log=true
 peekmode="saveWithPeek"
 run_tests_lo "$ns1" "$ns1" 10.0.1.1 1
-run_tests_lo "$ns1" $sender dead:beef:1::1 1
+run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1
 
 options_log=true
 peekmode="saveAfterPeek"
 run_tests_lo "$ns1" "$ns1" 10.0.1.1 1
-run_tests_lo "$ns1" $sender dead:beef:1::1 1
+run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1
 
 time_end=$(date +%s)
 time_run=$((time_end-time_start))
-- 
2.30.2

[PATCH mptcp-next 3/3] selftests: mptcp: avoid non-const global variables

[Matthieu Baerts]

In these scripts, we have "global variables" that are only set once when
parsing the config. That's fine, they are const.

It is often not recommended to use non-const global variables for
various reasons.

Here, we were changing the behaviour of a function by changing the value
of a global var just before calling this function and reset the global
var after for the next tests. Even if we are in a Bash script, best to
avoid this because that will certainly cause issues later.

Now we pass extra args for mptcp_connect directly through the functions
we call.

Also by avoiding code duplication, it allowed me to detect 2 issues, see
the previous patches.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 .../selftests/net/mptcp/mptcp_connect.sh      | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index dbb4aea8b636..9236609731b1 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -23,8 +23,6 @@ rcvbuf=0
 options_log=true
 do_tcp=0
 filesize=0
-peekmode=""
-testpeek=false
 
 if [ $tc_loss -eq 100 ];then
 	tc_loss=1%
@@ -377,7 +375,7 @@ do_transfer()
 	local srv_proto="$4"
 	local connect_addr="$5"
 	local local_addr="$6"
-	local extra_args=""
+	local extra_args="$7"
 
 	local port
 	port=$((10000+$TEST_COUNT))
@@ -395,14 +393,10 @@ do_transfer()
 		extra_args="$extra_args -m $testmode"
 	fi
 
-	if $testpeek; then
-		extra_args="$extra_args -P $peekmode"
-	fi
-
 	if [ -n "$extra_args" ] && $options_log; then
-		options_log=false
 		echo "INFO: extra options: $extra_args"
 	fi
+	options_log=false
 
 	:> "$cout"
 	:> "$sout"
@@ -595,6 +589,7 @@ run_tests_lo()
 	local connector_ns="$2"
 	local connect_addr="$3"
 	local loopback="$4"
+	local extra_args="$5"
 	local lret=0
 
 	# skip if test programs are running inside same netns for subsequent runs.
@@ -614,7 +609,8 @@ run_tests_lo()
 		local_addr="0.0.0.0"
 	fi
 
-	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr} ${local_addr}
+	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP \
+		    ${connect_addr} ${local_addr} "${extra_args}"
 	lret=$?
 	if [ $lret -ne 0 ]; then
 		ret=$lret
@@ -628,14 +624,16 @@ run_tests_lo()
 		fi
 	fi
 
-	do_transfer ${listener_ns} ${connector_ns} MPTCP TCP ${connect_addr} ${local_addr}
+	do_transfer ${listener_ns} ${connector_ns} MPTCP TCP \
+		    ${connect_addr} ${local_addr} "${extra_args}"
 	lret=$?
 	if [ $lret -ne 0 ]; then
 		ret=$lret
 		return 1
 	fi
 
-	do_transfer ${listener_ns} ${connector_ns} TCP MPTCP ${connect_addr} ${local_addr}
+	do_transfer ${listener_ns} ${connector_ns} TCP MPTCP \
+		    ${connect_addr} ${local_addr} "${extra_args}"
 	lret=$?
 	if [ $lret -ne 0 ]; then
 		ret=$lret
@@ -643,7 +641,8 @@ run_tests_lo()
 	fi
 
 	if [ $do_tcp -gt 1 ] ;then
-		do_transfer ${listener_ns} ${connector_ns} TCP TCP ${connect_addr} ${local_addr}
+		do_transfer ${listener_ns} ${connector_ns} TCP TCP \
+			    ${connect_addr} ${local_addr} "${extra_args}"
 		lret=$?
 		if [ $lret -ne 0 ]; then
 			ret=$lret
@@ -659,6 +658,15 @@ run_tests()
 	run_tests_lo $1 $2 $3 0
 }
 
+run_tests_peekmode()
+{
+	local peekmode="$1"
+
+	echo "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}"
+}
+
 make_file "$cin" "client"
 make_file "$sin" "server"
 
@@ -738,16 +746,8 @@ for sender in $ns1 $ns2 $ns3 $ns4;do
 	run_tests "$ns4" $sender dead:beef:3::1
 done
 
-testpeek=true
-options_log=true
-peekmode="saveWithPeek"
-run_tests_lo "$ns1" "$ns1" 10.0.1.1 1
-run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1
-
-options_log=true
-peekmode="saveAfterPeek"
-run_tests_lo "$ns1" "$ns1" 10.0.1.1 1
-run_tests_lo "$ns1" "$ns1" dead:beef:1::1 1
+run_tests_peekmode "saveWithPeek"
+run_tests_peekmode "saveAfterPeek"
 
 time_end=$(date +%s)
 time_run=$((time_end-time_start))
-- 
2.30.2

Re: [PATCH mptcp-next 0/3] Squash to "selftests: mptcp: add a test case for MSG_PEEK"

[Mat Martineau]

On Mon, 19 Apr 2021, Matthieu Baerts wrote:

> I started this because I prefer avoiding setting a global variable before
> calling a function and reset it later in order to take a specific path in the
> called function. While at it, I also reduced duplicated code.
>
> But good thing, I also fixed two unwanted behaviours, see patches 1/3 and 2/3.
>
> Matthieu Baerts (3):
>  selftests: mptcp: use 2 different 'peek' modes
>  selftests: mptcp: same NS for Peek tests in IPv6
>  selftests: mptcp: avoid non-const global variables
>
> .../selftests/net/mptcp/mptcp_connect.sh      | 44 +++++++++----------
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> -- 
> 2.30.2

Hi Matthieu -

These look good to squash, thanks for the fixups!


--
Mat Martineau
Intel

Re: [PATCH mptcp-next 0/3] Squash to "selftests: mptcp: add a test case for MSG_PEEK"

[Matthieu Baerts]

Hi Mat,

On 20/04/2021 00:28, Mat Martineau wrote:
> On Mon, 19 Apr 2021, Matthieu Baerts wrote:
> 
>> I started this because I prefer avoiding setting a global variable before
>> calling a function and reset it later in order to take a specific path
>> in the
>> called function. While at it, I also reduced duplicated code.
>>
>> But good thing, I also fixed two unwanted behaviours, see patches 1/3
>> and 2/3.
> 
> Hi Matthieu -
> 
> These look good to squash, thanks for the fixups!

Thank you for your review!

I just added these patches in our tree and I added your RvB tag to the
patch "selftests: mptcp: add a test case for MSG_PEEK". I hope that's
alright. I can remove it if you prefer. I guess you will replace it with
your SoB when sending them anyway :)

- 56b3332ce650: "squashed" patch 1/3 in "selftests: mptcp: add a test
case for MSG_PEEK"
- 2c2ca30de1d6: "Signed-off-by" + "Co-developed-by"
- 7d6c15c30c4c: "squashed" patch 2/3 in "selftests: mptcp: add a test
case for MSG_PEEK"
- 739a1c5fe1d6: "squashed" patch 3/3 in "selftests: mptcp: add a test
case for MSG_PEEK"
- ebcf2d3b0e42: tg:msg: add Mat's RvB tag
- Results: 04c5ff771be8..e69889ce0094

Tests + export are in progress!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net