[PATCH net 0/2] selftests: fix mptcp_join test

[PATCH net 0/2] selftests: fix mptcp_join test

[Andrea Claudi]

This series fixes two mptcp_join testcases.
- '001 implicit EP' fails because of:
  - missing iproute support for mptcp 'implicit' flag, fixed with
    iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit
    flag")
  - pm_nl_check_endpoint expecting two ip addresses, while only one is
    present in the iproute output;
- '002 delete and re-add' fails because the endpoint delete command
  provide both id and ip address, while address should be provided only
  if id is 0.

Andrea Claudi (2):
  selftests: mptcp: join: fix 'delete and re-add' test
  selftests: mptcp: join: fix 'implicit EP' test

 tools/testing/selftests/net/mptcp/mptcp_join.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH net 1/2] selftests: mptcp: join: fix 'delete and re-add' test

[Andrea Claudi]

mptcp_join '002 delete and re-add' test currently fails in the 'after
delete' testcase.

This happens because endpoint delete includes an ip address while id is
not 0, contrary to what is indicated in the ip mptcp man page:

"When used with the delete id operation, an IFADDR is only included when
the ID is 0."

This fixes the issue simply not using the $addr variable in
pm_nl_del_endpoint().

Fixes: 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 0ae8cafde439..5424dcacfffa 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -678,7 +678,7 @@ pm_nl_del_endpoint()
 	local addr=$3
 
 	if [ $ip_mptcp -eq 1 ]; then
-		ip -n $ns mptcp endpoint delete id $id $addr
+		ip -n $ns mptcp endpoint delete id $id
 	else
 		ip netns exec $ns ./pm_nl_ctl del $id $addr
 	fi
-- 
2.41.0

[PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

[Andrea Claudi]

mptcp_join '001 implicit EP' test currently fails because of two
reasons:

- iproute v6.3.0 does not support the implicit flag, fixed with
  iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit
  flag")
- pm_nl_check_endpoint wrongly expects the ip address to be repeated two
  times in iproute output, and does not account for a final whitespace
  in it.

This fixes the issue trimming the whitespace in the output string and
removing the double address in the expected string.

Fixes: 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 5424dcacfffa..6c3525e42273 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -768,10 +768,11 @@ pm_nl_check_endpoint()
 	fi
 
 	if [ $ip_mptcp -eq 1 ]; then
+		# get line and trim trailing whitespace
 		line=$(ip -n $ns mptcp endpoint show $id)
+		line="${line% }"
 		# the dump order is: address id flags port dev
-		expected_line="$addr"
-		[ -n "$addr" ] && expected_line="$expected_line $addr"
+		[ -n "$addr" ] && expected_line="$addr"
 		expected_line="$expected_line $id"
 		[ -n "$_flags" ] && expected_line="$expected_line ${_flags//","/" "}"
 		[ -n "$dev" ] && expected_line="$expected_line $dev"
-- 
2.41.0

Re: [PATCH net 0/2] selftests: fix mptcp_join test

[Matthieu Baerts]

Hi Andrea,

On 23/06/2023 14:19, Andrea Claudi wrote:
> This series fixes two mptcp_join testcases.
> - '001 implicit EP' fails because of:
>   - missing iproute support for mptcp 'implicit' flag, fixed with
>     iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit
>     flag")
>   - pm_nl_check_endpoint expecting two ip addresses, while only one is
>     present in the iproute output;
> - '002 delete and re-add' fails because the endpoint delete command
>   provide both id and ip address, while address should be provided only
>   if id is 0.
> 
> Andrea Claudi (2):
>   selftests: mptcp: join: fix 'delete and re-add' test
>   selftests: mptcp: join: fix 'implicit EP' test

Thank you for these patches!

I have some comments, please see my replies on the individual patches.

Do you mind sending a v2 only to MPTCP ML -- not to Netdev and its
maintainers -- if you don't mind? When these patches will be ready, we
will apply them in MPTCP tree and send them later to netdev.

(Maybe we should modify the MAINTAINERS file to exclude Netdev
maintainers and list when someone sends a patch.)

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

Re: [PATCH net 1/2] selftests: mptcp: join: fix 'delete and re-add' test

[Matthieu Baerts]

Hi Andrea,

On 23/06/2023 14:19, Andrea Claudi wrote:
> mptcp_join '002 delete and re-add' test currently fails in the 'after
> delete' testcase.

I guess it only fails if you use "-i" option to use "ip mptcp" instead
of "pm_nl_ctl", right?

MPTCP CI doesn't launch the tests with the "-i" option.

Can you mention that it fails only when using "ip mptcp" which is not
the default mode please? It might be good to include that in the title
too not to think the test is broken and the CI didn't complain about that.

BTW, how did you launch mptcp_join.sh selftest to have this test
launched as second position ("002")? With "-Ii"?

(you can remove this "002": it is specific to the way you launched the
test, not using the default mode)

> This happens because endpoint delete includes an ip address while id is
> not 0, contrary to what is indicated in the ip mptcp man page:
> 
> "When used with the delete id operation, an IFADDR is only included when
> the ID is 0."
> 
> This fixes the issue simply not using the $addr variable in
> pm_nl_del_endpoint().

If you do that, are you not going to break other tests? e.g.
- "remove id 0 subflow"
- "remove id 0 address"

(I didn't check all possibilities, maybe not or maybe there are others)

Because if you specify the ID 0, you do need to specify the address, no?

> Fixes: 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 0ae8cafde439..5424dcacfffa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -678,7 +678,7 @@ pm_nl_del_endpoint()
>  	local addr=$3
>  
>  	if [ $ip_mptcp -eq 1 ]; then
> -		ip -n $ns mptcp endpoint delete id $id $addr
> +		ip -n $ns mptcp endpoint delete id $id

Should you not add "${addr}" only if ${id} == 1?

>  	else
>  		ip netns exec $ns ./pm_nl_ctl del $id $addr
>  	fi

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

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

[Matthieu Baerts]

Hi Andrea,

On 23/06/2023 14:19, Andrea Claudi wrote:
> mptcp_join '001 implicit EP' test currently fails because of two
> reasons:

Same as on patch 1/2: can you remove "001" and mention it is only
failing when using "ip mptcp" ("-i" option) please?

> - iproute v6.3.0 does not support the implicit flag, fixed with
>   iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit
>   flag")

Thank you for that!

Out of curiosity: why is it in iproute2-next (following net-next tree,
for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?

> - pm_nl_check_endpoint wrongly expects the ip address to be repeated two
>   times in iproute output, and does not account for a final whitespace
>   in it.
> 
> This fixes the issue trimming the whitespace in the output string and
> removing the double address in the expected string.
> 
> Fixes: 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 5424dcacfffa..6c3525e42273 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -768,10 +768,11 @@ pm_nl_check_endpoint()
>  	fi
>  
>  	if [ $ip_mptcp -eq 1 ]; then
> +		# get line and trim trailing whitespace
>  		line=$(ip -n $ns mptcp endpoint show $id)
> +		line="${line% }"
>  		# the dump order is: address id flags port dev
> -		expected_line="$addr"
> -		[ -n "$addr" ] && expected_line="$expected_line $addr"
> +		[ -n "$addr" ] && expected_line="$addr"
>  		expected_line="$expected_line $id"
>  		[ -n "$_flags" ] && expected_line="$expected_line ${_flags//","/" "}"
>  		[ -n "$dev" ] && expected_line="$expected_line $dev"

It looks good, no need to change anything here but later (not for -net
anyway), we should probably parse the JSON output of "ip -j mptcp" instead.

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

Re: [PATCH net 1/2] selftests: mptcp: join: fix 'delete and re-add' test

[Andrea Claudi]

On Mon, Jun 26, 2023 at 01:31:07PM +0200, Matthieu Baerts wrote:
> Hi Andrea,

Hi Matthieu,
Thanks for your review.

> 
> On 23/06/2023 14:19, Andrea Claudi wrote:
> > mptcp_join '002 delete and re-add' test currently fails in the 'after
> > delete' testcase.
> 
> I guess it only fails if you use "-i" option to use "ip mptcp" instead
> of "pm_nl_ctl", right?
>

Yes, exactly.

> MPTCP CI doesn't launch the tests with the "-i" option.
> 
> Can you mention that it fails only when using "ip mptcp" which is not
> the default mode please? It might be good to include that in the title
> too not to think the test is broken and the CI didn't complain about that.
>

Sure, will do that.

> BTW, how did you launch mptcp_join.sh selftest to have this test
> launched as second position ("002")? With "-Ii"?
> 

Yes, that's exactly the case, I use "./mptcp_join.sh -I -i"

> (you can remove this "002": it is specific to the way you launched the
> test, not using the default mode)

Will do.

> 
> > This happens because endpoint delete includes an ip address while id is
> > not 0, contrary to what is indicated in the ip mptcp man page:
> > 
> > "When used with the delete id operation, an IFADDR is only included when
> > the ID is 0."
> > 
> > This fixes the issue simply not using the $addr variable in
> > pm_nl_del_endpoint().
> 
> If you do that, are you not going to break other tests? e.g.
> - "remove id 0 subflow"
> - "remove id 0 address"
> 
> (I didn't check all possibilities, maybe not or maybe there are others)
> 
> Because if you specify the ID 0, you do need to specify the address, no?
> 

That's right, of course. I'll fix that in v2 and make sure no other
tests are impacted with a "mptcp_join.sh -i" run.

> > Fixes: 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 0ae8cafde439..5424dcacfffa 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -678,7 +678,7 @@ pm_nl_del_endpoint()
> >  	local addr=$3
> >  
> >  	if [ $ip_mptcp -eq 1 ]; then
> > -		ip -n $ns mptcp endpoint delete id $id $addr
> > +		ip -n $ns mptcp endpoint delete id $id
> 
> Should you not add "${addr}" only if ${id} == 1?
> 
> >  	else
> >  		ip netns exec $ns ./pm_nl_ctl del $id $addr
> >  	fi
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

[Andrea Claudi]

On Mon, Jun 26, 2023 at 01:32:17PM +0200, Matthieu Baerts wrote:
> Hi Andrea,
> 
> On 23/06/2023 14:19, Andrea Claudi wrote:
> > mptcp_join '001 implicit EP' test currently fails because of two
> > reasons:
> 
> Same as on patch 1/2: can you remove "001" and mention it is only
> failing when using "ip mptcp" ("-i" option) please?
>

Sure, no problem.

> > - iproute v6.3.0 does not support the implicit flag, fixed with
> >   iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit
> >   flag")
> 
> Thank you for that!
> 
> Out of curiosity: why is it in iproute2-next (following net-next tree,
> for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?
> 

I usually target fixes to iproute2 and new stuff to iproute2-next, no
other reason than that. But I see your point here, having this on -net
may end up in the commit not landing in the same release cycle.

Should I send v2 for this series to mptcp-next, then?

Andrea

> > - pm_nl_check_endpoint wrongly expects the ip address to be repeated two
> >   times in iproute output, and does not account for a final whitespace
> >   in it.
> > 
> > This fixes the issue trimming the whitespace in the output string and
> > removing the double address in the expected string.
> > 
> > Fixes: 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 5424dcacfffa..6c3525e42273 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -768,10 +768,11 @@ pm_nl_check_endpoint()
> >  	fi
> >  
> >  	if [ $ip_mptcp -eq 1 ]; then
> > +		# get line and trim trailing whitespace
> >  		line=$(ip -n $ns mptcp endpoint show $id)
> > +		line="${line% }"
> >  		# the dump order is: address id flags port dev
> > -		expected_line="$addr"
> > -		[ -n "$addr" ] && expected_line="$expected_line $addr"
> > +		[ -n "$addr" ] && expected_line="$addr"
> >  		expected_line="$expected_line $id"
> >  		[ -n "$_flags" ] && expected_line="$expected_line ${_flags//","/" "}"
> >  		[ -n "$dev" ] && expected_line="$expected_line $dev"
> 
> It looks good, no need to change anything here but later (not for -net
> anyway), we should probably parse the JSON output of "ip -j mptcp" instead.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

[Matthieu Baerts]

Hi Andrea,

Thank you for your replies!

On 27/06/2023 15:43, Andrea Claudi wrote:
> On Mon, Jun 26, 2023 at 01:32:17PM +0200, Matthieu Baerts wrote:
>> On 23/06/2023 14:19, Andrea Claudi wrote:

(...)

>> Out of curiosity: why is it in iproute2-next (following net-next tree,
>> for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?
>>
> 
> I usually target fixes to iproute2 and new stuff to iproute2-next, no
> other reason than that. But I see your point here, having this on -net
> may end up in the commit not landing in the same release cycle.

I see, thank you for the explanation.

If I'm not mistaken, a big difference with how the 'net' tree is handled
-- i.e. only bug fixes -- 'iproute2' tree accepts new features as long
as the kernel using the 'net' tree supports these new features. If it
depends on features only in 'net-next', then the patches should target
'iproute2-next'.

> Should I send v2 for this series to mptcp-next, then?

Yes please, only to the MPTCP list without netdev list and maintainers
if you don't mind, just to avoid bothering too many people with MPTCP
specific stuff :)

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