[v1] selftests: fix mptcp_join test

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

Posted by Andrea Claudi 2 years, 7 months ago

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 2/2] selftests: mptcp: join: fix 'implicit EP' test

Posted by Matthieu Baerts 2 years, 7 months ago

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 2/2] selftests: mptcp: join: fix 'implicit EP' test

Posted by Andrea Claudi 2 years, 7 months ago

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

Posted by Matthieu Baerts 2 years, 7 months ago

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