[v5] mptcp: fix endpoints with 'signal' and 'subflow' flags

[PATCH mptcp-net v5 09/13] selftests: mptcp: join: validate fullmesh endp on 1st sf

Posted by Matthieu Baerts (NGI0) 1 month, 3 weeks ago

This case was not covered, and the wrong ID was set before the previous
commit.

The rest is not modified, it is just that it will increase the code
coverage.

The right address ID can be verified by looking at the packet traces. We
could automate that using Netfilter with some cBPF code for example, but
that's always a bit cryptic. Packetdrill seems better fitted for that.

Fixes: 4f49d63352da ("selftests: mptcp: add fullmesh testcases")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f609c02c6123..e4ac275366ce 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3058,6 +3058,7 @@ fullmesh_tests()
 	if reset "fullmesh test 1x1"; then
 		pm_nl_set_limits $ns1 1 3
 		pm_nl_set_limits $ns2 1 3
+		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,fullmesh
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
 		fullmesh=1 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1

-- 
2.45.2

Re: [PATCH mptcp-net v5 09/13] selftests: mptcp: join: validate fullmesh endp on 1st sf

Posted by Geliang Tang 1 month, 2 weeks ago

On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
> This case was not covered, and the wrong ID was set before the
> previous
> commit.
> 
> The rest is not modified, it is just that it will increase the code
> coverage.
> 
> The right address ID can be verified by looking at the packet traces.
> We
> could automate that using Netfilter with some cBPF code for example,
> but
> that's always a bit cryptic. Packetdrill seems better fitted for
> that.
> 
> Fixes: 4f49d63352da ("selftests: mptcp: add fullmesh testcases")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index f609c02c6123..e4ac275366ce 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3058,6 +3058,7 @@ fullmesh_tests()
>  	if reset "fullmesh test 1x1"; then
>  		pm_nl_set_limits $ns1 1 3
>  		pm_nl_set_limits $ns2 1 3
> +		pm_nl_add_endpoint $ns2 10.0.1.2 flags
> subflow,fullmesh

nit: add this line behind "pm_nl_add_endpoint $ns1 10.0.2.1 flags
signal". Usually, set n1 first and then n2.

>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>  		fullmesh=1 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>

Re: [PATCH mptcp-net v5 09/13] selftests: mptcp: join: validate fullmesh endp on 1st sf

Posted by Matthieu Baerts 1 month, 2 weeks ago

Hi Geliang,

On 29/07/2024 03:19, Geliang Tang wrote:
> On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
>> This case was not covered, and the wrong ID was set before the
>> previous
>> commit.
>>
>> The rest is not modified, it is just that it will increase the code
>> coverage.
>>
>> The right address ID can be verified by looking at the packet traces.
>> We
>> could automate that using Netfilter with some cBPF code for example,
>> but
>> that's always a bit cryptic. Packetdrill seems better fitted for
>> that.
>>
>> Fixes: 4f49d63352da ("selftests: mptcp: add fullmesh testcases")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index f609c02c6123..e4ac275366ce 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -3058,6 +3058,7 @@ fullmesh_tests()
>>  	if reset "fullmesh test 1x1"; then
>>  		pm_nl_set_limits $ns1 1 3
>>  		pm_nl_set_limits $ns2 1 3
>> +		pm_nl_add_endpoint $ns2 10.0.1.2 flags
>> subflow,fullmesh
> 
> nit: add this line behind "pm_nl_add_endpoint $ns1 10.0.2.1 flags
> signal". Usually, set n1 first and then n2.

It looks like we have a mix, but yes, most of the time we set the
endpoints for the ns1, then the ns2. I can fix that when applying the
patches if there are no other big changes needed here.

>>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>>  		fullmesh=1 speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>>
> 
> 

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

Re: [PATCH mptcp-net v5 09/13] selftests: mptcp: join: validate fullmesh endp on 1st sf

Posted by Geliang Tang 1 month, 2 weeks ago

On Mon, 2024-07-29 at 11:35 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/07/2024 03:19, Geliang Tang wrote:
> > On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
> > > This case was not covered, and the wrong ID was set before the
> > > previous
> > > commit.
> > > 
> > > The rest is not modified, it is just that it will increase the
> > > code
> > > coverage.
> > > 
> > > The right address ID can be verified by looking at the packet
> > > traces.
> > > We
> > > could automate that using Netfilter with some cBPF code for
> > > example,
> > > but
> > > that's always a bit cryptic. Packetdrill seems better fitted for
> > > that.
> > > 
> > > Fixes: 4f49d63352da ("selftests: mptcp: add fullmesh testcases")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index f609c02c6123..e4ac275366ce 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -3058,6 +3058,7 @@ fullmesh_tests()
> > >  	if reset "fullmesh test 1x1"; then
> > >  		pm_nl_set_limits $ns1 1 3
> > >  		pm_nl_set_limits $ns2 1 3
> > > +		pm_nl_add_endpoint $ns2 10.0.1.2 flags
> > > subflow,fullmesh
> > 
> > nit: add this line behind "pm_nl_add_endpoint $ns1 10.0.2.1 flags
> > signal". Usually, set n1 first and then n2.
> 
> It looks like we have a mix, but yes, most of the time we set the
> endpoints for the ns1, then the ns2. I can fix that when applying the
> patches if there are no other big changes needed here.

Sure, please update this when merging it.

> 
> > >  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> > >  		fullmesh=1 speed=slow \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > > 
> > 
> > 
> 
> Cheers,
> Matt