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

[Andrea Claudi <aclaudi@redhat.com>]

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
>