In the following commit, the client will initiate the ADD_ADDR, instead
of the server. We need to way to verify the ADD_ADDR have been correctly
sent.
Note: the default expected counters for when the port number is given
are never changed by the caller, no need to accept them as parameter
then.
The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.
Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
tools/testing/selftests/net/mptcp/mptcp_join.sh | 40 ++++++++++++++++---------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 55d84a1bde15..55ccc4fdf18a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1415,18 +1415,28 @@ chk_add_nr()
local add_nr=$1
local echo_nr=$2
local port_nr=${3:-0}
- local syn_nr=${4:-$port_nr}
- local syn_ack_nr=${5:-$port_nr}
- local ack_nr=${6:-$port_nr}
- local mis_syn_nr=${7:-0}
- local mis_ack_nr=${8:-0}
+ local ns_invert=${4:-""}
+ local syn_nr=$port_nr
+ local syn_ack_nr=$port_nr
+ local ack_nr=$port_nr
+ local mis_syn_nr=0
+ local mis_ack_nr=0
+ local ns_tx=$ns1
+ local ns_rx=$ns2
+ local extra_msg=""
local count
local timeout
- timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
+ if [[ $ns_invert = "invert" ]]; then
+ ns_tx=$ns2
+ ns_rx=$ns1
+ extra_msg="invert"
+ fi
+
+ timeout=$(ip netns exec ${ns_tx} sysctl -n net.mptcp.add_addr_timeout)
print_check "add"
- count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtAddAddr")
+ count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
if [ -z "$count" ]; then
print_skip
# if the test configured a short timeout tolerate greater then expected
@@ -1438,7 +1448,7 @@ chk_add_nr()
fi
print_check "echo"
- count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtEchoAdd")
+ count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$echo_nr" ]; then
@@ -1449,7 +1459,7 @@ chk_add_nr()
if [ $port_nr -gt 0 ]; then
print_check "pt"
- count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtPortAdd")
+ count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtPortAdd")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$port_nr" ]; then
@@ -1459,7 +1469,7 @@ chk_add_nr()
fi
print_check "syn"
- count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortSynRx")
+ count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortSynRx")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$syn_nr" ]; then
@@ -1470,7 +1480,7 @@ chk_add_nr()
fi
print_check "synack"
- count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinPortSynAckRx")
+ count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPJoinPortSynAckRx")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$syn_ack_nr" ]; then
@@ -1481,7 +1491,7 @@ chk_add_nr()
fi
print_check "ack"
- count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortAckRx")
+ count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortAckRx")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$ack_nr" ]; then
@@ -1492,7 +1502,7 @@ chk_add_nr()
fi
print_check "syn"
- count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortSynRx")
+ count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortSynRx")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$mis_syn_nr" ]; then
@@ -1503,7 +1513,7 @@ chk_add_nr()
fi
print_check "ack"
- count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortAckRx")
+ count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortAckRx")
if [ -z "$count" ]; then
print_skip
elif [ "$count" != "$mis_ack_nr" ]; then
@@ -1513,6 +1523,8 @@ chk_add_nr()
print_ok
fi
fi
+
+ print_info "$extra_msg"
}
chk_add_tx_nr()
--
2.45.2Hi Matt,
On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
> In the following commit, the client will initiate the ADD_ADDR,
> instead
> of the server. We need to way to verify the ADD_ADDR have been
> correctly
> sent.
>
> Note: the default expected counters for when the port number is given
> are never changed by the caller, no need to accept them as parameter
> then.
>
> The 'Fixes' tag here below is the same as the one from the previous
> commit: this patch here is not fixing anything wrong in the
> selftests,
> but it validates the previous fix for an issue introduced by this
> commit
> ID.
>
> Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still
> available for each msk")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 40
> ++++++++++++++++---------
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 55d84a1bde15..55ccc4fdf18a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1415,18 +1415,28 @@ chk_add_nr()
> local add_nr=$1
> local echo_nr=$2
> local port_nr=${3:-0}
> - local syn_nr=${4:-$port_nr}
> - local syn_ack_nr=${5:-$port_nr}
> - local ack_nr=${6:-$port_nr}
> - local mis_syn_nr=${7:-0}
> - local mis_ack_nr=${8:-0}
nit: How about moving the code that reduces arguments into a separate
patch?
> + local ns_invert=${4:-""}
> + local syn_nr=$port_nr
> + local syn_ack_nr=$port_nr
> + local ack_nr=$port_nr
> + local mis_syn_nr=0
> + local mis_ack_nr=0
> + local ns_tx=$ns1
> + local ns_rx=$ns2
And move the code for renaming ns into another separate patch too?
WDYT?
Thanks,
-Geliang
> + local extra_msg=""
> local count
> local timeout
>
> - timeout=$(ip netns exec $ns1 sysctl -n
> net.mptcp.add_addr_timeout)
> + if [[ $ns_invert = "invert" ]]; then
> + ns_tx=$ns2
> + ns_rx=$ns1
> + extra_msg="invert"
> + fi
> +
> + timeout=$(ip netns exec ${ns_tx} sysctl -n
> net.mptcp.add_addr_timeout)
>
> print_check "add"
> - count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtAddAddr")
> + count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
> if [ -z "$count" ]; then
> print_skip
> # if the test configured a short timeout tolerate greater
> then expected
> @@ -1438,7 +1448,7 @@ chk_add_nr()
> fi
>
> print_check "echo"
> - count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtEchoAdd")
> + count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$echo_nr" ]; then
> @@ -1449,7 +1459,7 @@ chk_add_nr()
>
> if [ $port_nr -gt 0 ]; then
> print_check "pt"
> - count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtPortAdd")
> + count=$(mptcp_lib_get_counter ${ns_rx}
> "MPTcpExtPortAdd")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$port_nr" ]; then
> @@ -1459,7 +1469,7 @@ chk_add_nr()
> fi
>
> print_check "syn"
> - count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMPJoinPortSynRx")
> + count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMPJoinPortSynRx")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$syn_nr" ]; then
> @@ -1470,7 +1480,7 @@ chk_add_nr()
> fi
>
> print_check "synack"
> - count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinPortSynAckRx")
> + count=$(mptcp_lib_get_counter ${ns_rx}
> "MPTcpExtMPJoinPortSynAckRx")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$syn_ack_nr" ]; then
> @@ -1481,7 +1491,7 @@ chk_add_nr()
> fi
>
> print_check "ack"
> - count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMPJoinPortAckRx")
> + count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMPJoinPortAckRx")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$ack_nr" ]; then
> @@ -1492,7 +1502,7 @@ chk_add_nr()
> fi
>
> print_check "syn"
> - count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMismatchPortSynRx")
> + count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMismatchPortSynRx")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$mis_syn_nr" ]; then
> @@ -1503,7 +1513,7 @@ chk_add_nr()
> fi
>
> print_check "ack"
> - count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMismatchPortAckRx")
> + count=$(mptcp_lib_get_counter ${ns_tx}
> "MPTcpExtMismatchPortAckRx")
> if [ -z "$count" ]; then
> print_skip
> elif [ "$count" != "$mis_ack_nr" ]; then
> @@ -1513,6 +1523,8 @@ chk_add_nr()
> print_ok
> fi
> fi
> +
> + print_info "$extra_msg"
> }
>
> chk_add_tx_nr()
>
Hi Geliang,
Thank you for this review!
On 16/07/2024 05:20, Geliang Tang wrote:
> Hi Matt,
>
> On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
>> In the following commit, the client will initiate the ADD_ADDR,
>> instead
>> of the server. We need to way to verify the ADD_ADDR have been
>> correctly
>> sent.
>>
>> Note: the default expected counters for when the port number is given
>> are never changed by the caller, no need to accept them as parameter
>> then.
>>
>> The 'Fixes' tag here below is the same as the one from the previous
>> commit: this patch here is not fixing anything wrong in the
>> selftests,
>> but it validates the previous fix for an issue introduced by this
>> commit
>> ID.
>>
>> Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still
>> available for each msk")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 40
>> ++++++++++++++++---------
>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 55d84a1bde15..55ccc4fdf18a 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1415,18 +1415,28 @@ chk_add_nr()
>> local add_nr=$1
>> local echo_nr=$2
>> local port_nr=${3:-0}
>> - local syn_nr=${4:-$port_nr}
>> - local syn_ack_nr=${5:-$port_nr}
>> - local ack_nr=${6:-$port_nr}
>> - local mis_syn_nr=${7:-0}
>> - local mis_ack_nr=${8:-0}
>
> nit: How about moving the code that reduces arguments into a separate
> patch?
I prefer not to in this case. If the patch was for net-next, that would
have been a good idea. Here, it is for -net, and backporting shell
scripts is a pain: there might not be any conflicts, but the script
execution can be easily broken, and the stable team will not notice it
because they don't execute them. Here, I want everything or nothing to
be backported, not half of it. I even wonder if it would not be easier
to squash patch 6 and 7 together. But maybe not for the moment to ease
the reviews. WDYT?
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On Tue, 2024-07-16 at 10:12 +0200, Matthieu Baerts wrote:
> Hi Geliang,
>
> Thank you for this review!
>
> On 16/07/2024 05:20, Geliang Tang wrote:
> > Hi Matt,
> >
> > On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
> > > In the following commit, the client will initiate the ADD_ADDR,
> > > instead
> > > of the server. We need to way to verify the ADD_ADDR have been
> > > correctly
> > > sent.
> > >
> > > Note: the default expected counters for when the port number is
> > > given
> > > are never changed by the caller, no need to accept them as
> > > parameter
> > > then.
> > >
> > > The 'Fixes' tag here below is the same as the one from the
> > > previous
> > > commit: this patch here is not fixing anything wrong in the
> > > selftests,
> > > but it validates the previous fix for an issue introduced by this
> > > commit
> > > ID.
> > >
> > > Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still
> > > available for each msk")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > tools/testing/selftests/net/mptcp/mptcp_join.sh | 40
> > > ++++++++++++++++---------
> > > 1 file changed, 26 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index 55d84a1bde15..55ccc4fdf18a 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1415,18 +1415,28 @@ chk_add_nr()
> > > local add_nr=$1
> > > local echo_nr=$2
> > > local port_nr=${3:-0}
> > > - local syn_nr=${4:-$port_nr}
> > > - local syn_ack_nr=${5:-$port_nr}
> > > - local ack_nr=${6:-$port_nr}
> > > - local mis_syn_nr=${7:-0}
> > > - local mis_ack_nr=${8:-0}
> >
> > nit: How about moving the code that reduces arguments into a
> > separate
> > patch?
>
> I prefer not to in this case. If the patch was for net-next, that
> would
> have been a good idea. Here, it is for -net, and backporting shell
> scripts is a pain: there might not be any conflicts, but the script
How about this, send all these selftests patches in this set to net-
next, and the rest to -net. After all, these selftests patches are not
fixes, and selftests in -net can run normally without them, right?
But up to you.
Thanks,
-Geliang
> execution can be easily broken, and the stable team will not notice
> it
> because they don't execute them. Here, I want everything or nothing
> to
> be backported, not half of it. I even wonder if it would not be
> easier
> to squash patch 6 and 7 together. But maybe not for the moment to
> ease
> the reviews. WDYT?
>
> Cheers,
> Matt
Hi Geliang,
On 17/07/2024 05:53, Geliang Tang wrote:
> On Tue, 2024-07-16 at 10:12 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for this review!
>>
>> On 16/07/2024 05:20, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Mon, 2024-07-15 at 12:09 +0200, Matthieu Baerts (NGI0) wrote:
>>>> In the following commit, the client will initiate the ADD_ADDR,
>>>> instead
>>>> of the server. We need to way to verify the ADD_ADDR have been
>>>> correctly
>>>> sent.
>>>>
>>>> Note: the default expected counters for when the port number is
>>>> given
>>>> are never changed by the caller, no need to accept them as
>>>> parameter
>>>> then.
>>>>
>>>> The 'Fixes' tag here below is the same as the one from the
>>>> previous
>>>> commit: this patch here is not fixing anything wrong in the
>>>> selftests,
>>>> but it validates the previous fix for an issue introduced by this
>>>> commit
>>>> ID.
>>>>
>>>> Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still
>>>> available for each msk")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 40
>>>> ++++++++++++++++---------
>>>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> index 55d84a1bde15..55ccc4fdf18a 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -1415,18 +1415,28 @@ chk_add_nr()
>>>> local add_nr=$1
>>>> local echo_nr=$2
>>>> local port_nr=${3:-0}
>>>> - local syn_nr=${4:-$port_nr}
>>>> - local syn_ack_nr=${5:-$port_nr}
>>>> - local ack_nr=${6:-$port_nr}
>>>> - local mis_syn_nr=${7:-0}
>>>> - local mis_ack_nr=${8:-0}
>>>
>>> nit: How about moving the code that reduces arguments into a
>>> separate
>>> patch?
>>
>> I prefer not to in this case. If the patch was for net-next, that
>> would
>> have been a good idea. Here, it is for -net, and backporting shell
>> scripts is a pain: there might not be any conflicts, but the script
>
> How about this, send all these selftests patches in this set to net-
> next, and the rest to -net. After all, these selftests patches are not
> fixes, and selftests in -net can run normally without them, right?
That could simplify things, but I think it is important to attach the
tests to the fixes: it helps the reviewers to understand what was wrong,
and helps testers and maintainers to validate the new fixes, and make
sure there are no new regressions later. These commits should be
backported, but we should not spend too much time on that if there are
conflicts.
I could move this patch 6/17 to -next only, but that would make thing
even more complicated.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.