[PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses'

[PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses'

[Matthieu Baerts]

With Paolo, we were trying to understand why one test was failing, see:

https://github.com/multipath-tcp/mptcp_net-next/issues/254

Paolo noticed that even if the CI was reporting this:

  17 signal invalid addresses             syn[ ok ] - synack[ ok ] - ack[ ok ]
                                          add[ ok ] - echo  [ ok ]
                                          add[fail] got 2 ADD_ADDR[s] expected 3

The issue here was not with the 17th test but with the next one. This patch can
be sent to -net.

While at it, we can also fully remove mptcp_parse_option() from mptcp.h. Not
sure this patch needs to be sent to -net but it should not be rejected from
there :)

Matthieu Baerts (2):
  mptcp: mptcp_parse_option is no longer exported
  selftests: mptcp: add missing join check

 include/net/mptcp.h                             | 6 ------
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 2 files changed, 1 insertion(+), 6 deletions(-)

-- 
2.33.1

[PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported

[Matthieu Baerts]

Options parsing in now done from mptcp_incoming_options().

mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
defined but not when it is not.

Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 include/net/mptcp.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 04f53352a1c9..8b1afd6f5cc4 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
 	return false;
 }
 
-static inline void mptcp_parse_option(const struct sk_buff *skb,
-				      const unsigned char *ptr, int opsize,
-				      struct tcp_options_received *opt_rx)
-{
-}
-
 static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
 				     unsigned int *size,
 				     struct mptcp_out_options *opts)
-- 
2.33.1

[PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Matthieu Baerts]

This function also writes the name of the test with its ID, making clear
a new test has been executed.

Without that, the ADD_ADDR results from this test was appended at the
end of the previous test causing confusions. Especially when the second
test was failing, we had:

  17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
                                  add[ ok ] - echo  [ ok ]
                                  add[fail] got 2 ADD_ADDR[s] expected 3

In fact, this 17th test was OK but not the 18th one.

Now we have:

  17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
                                  add[ ok ] - echo  [ ok ]
  18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
                                  add[fail] got 2 ADD_ADDR[s] expected 3

Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 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 76c45c845d3b..67be3e1215d7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1342,6 +1342,7 @@ signal_address_tests()
 	pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
 	pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
 	run_tests $ns1 $ns2 10.0.1.1
+	chk_join_nr "signal addresses race test" 3 3 3
 
 	# the server will not signal the address terminating
 	# the MPC subflow
-- 
2.33.1

Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Geliang Tang]

Hi Matt,

I think in this test chk_join_nr is omitted on purpose, since it is more
likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
chk_join_nr, it will fail more frequently.

Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>
> This function also writes the name of the test with its ID, making clear
> a new test has been executed.
>
> Without that, the ADD_ADDR results from this test was appended at the
> end of the previous test causing confusions. Especially when the second
> test was failing, we had:
>
>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                   add[ ok ] - echo  [ ok ]
>                                   add[fail] got 2 ADD_ADDR[s] expected 3
>
> In fact, this 17th test was OK but not the 18th one.
>
> Now we have:
>
>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                   add[ ok ] - echo  [ ok ]
>   18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                   add[fail] got 2 ADD_ADDR[s] expected 3

I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
translated after ADD_ADDR, it may be lost.

>
> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  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 76c45c845d3b..67be3e1215d7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1342,6 +1342,7 @@ signal_address_tests()
>         pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
>         pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>         run_tests $ns1 $ns2 10.0.1.1
> +       chk_join_nr "signal addresses race test" 3 3 3

I think we can omit the join numbers check, just print out the name of the
test. Don't pass the join numbers to chk_join_nr, just the msg:

        chk_join_nr "signal addresses race test"

Then in chk_join_nr, test the input numbers before doing the numbers check:

'''
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -918,7 +918,7 @@ chk_join_nr()
     printf "%02u %-36s %s" "$TEST_COUNT" "$msg" "syn"
     count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinSynRx |
awk '{print $2}'`
     [ -z "$count" ] && count=0
-    if [ "$count" != "$syn_nr" ]; then
+    if [ -n "$syn_nr" ] && [ "$count" != "$syn_nr" ]; then
         echo "[fail] got $count JOIN[s] syn expected $syn_nr"
         ret=1
         dump_stats=1
@@ -929,7 +929,7 @@ chk_join_nr()
     echo -n " - synack"
     count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPJoinSynAckRx
| awk '{print $2}'`
     [ -z "$count" ] && count=0
-    if [ "$count" != "$syn_ack_nr" ]; then
+    if [ -n "$syn_ack_nr" ] && [ "$count" != "$syn_ack_nr" ]; then
         echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
         ret=1
         dump_stats=1
@@ -940,7 +940,7 @@ chk_join_nr()
     echo -n " - ack"
     count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinAckRx |
awk '{print $2}'`
     [ -z "$count" ] && count=0
-    if [ "$count" != "$ack_nr" ]; then
+    if [ -n ""$ack_nr"" ] && [ "$count" != "$ack_nr" ]; then
         echo "[fail] got $count JOIN[s] ack expected $ack_nr"
         ret=1
         dump_stats=1
'''

WDYT?

Thanks,
-Geliang

>
>         # the server will not signal the address terminating
>         # the MPC subflow
> --
> 2.33.1
>
>

Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Matthieu Baerts]

Hi Geliang,

Thank you for looking at that!

On 22/01/2022 15:04, Geliang Tang wrote:
> I think in this test chk_join_nr is omitted on purpose, since it is more
> likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
> chk_join_nr, it will fail more frequently.

May you explain why this tests is likely to fail? Both the commit
message and the comment in the code don't explain that.

> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>>
>> This function also writes the name of the test with its ID, making clear
>> a new test has been executed.
>>
>> Without that, the ADD_ADDR results from this test was appended at the
>> end of the previous test causing confusions. Especially when the second
>> test was failing, we had:
>>
>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>                                   add[ ok ] - echo  [ ok ]
>>                                   add[fail] got 2 ADD_ADDR[s] expected 3
>>
>> In fact, this 17th test was OK but not the 18th one.
>>
>> Now we have:
>>
>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>                                   add[ ok ] - echo  [ ok ]
>>   18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
>>                                   add[fail] got 2 ADD_ADDR[s] expected 3
> 
> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
> translated after ADD_ADDR, it may be lost.

Indeed, to simplify the commit message, I mixed both a "success" and a
"failure" output. I should not have done that and only put a "success"
one to keep the output small.

>> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>  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 76c45c845d3b..67be3e1215d7 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1342,6 +1342,7 @@ signal_address_tests()
>>         pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
>>         pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>>         run_tests $ns1 $ns2 10.0.1.1
>> +       chk_join_nr "signal addresses race test" 3 3 3
> 
> I think we can omit the join numbers check, just print out the name of the
> test. Don't pass the join numbers to chk_join_nr, just the msg:
> 
>         chk_join_nr "signal addresses race test"

Do you mean we could have situations where ADD_ADDR have been properly
sent and received but we check counters just before MPJ are emitted? In
this case we would mark the test as failed because MPJs are not OK
(checked too soon in fact) while ADD_ADDR are?

If yes and if it helps, we can also wait for the kernel to have sent
exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
already cover this UC where we look for MPJs emitted after having
received an ADD_ADDR, no?

If we keep checking all counters, I think it might be clearer: MPJ
counters are not OK because we didn't receive/process the ADD_ADDR, no?

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

Re: [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported

[Paolo Abeni]

On Sat, 2022-01-22 at 10:31 +0100, Matthieu Baerts wrote:
> Options parsing in now done from mptcp_incoming_options().
> 
> mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
> defined but not when it is not.
> 
> Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  include/net/mptcp.h | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 04f53352a1c9..8b1afd6f5cc4 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
>  	return false;
>  }
>  
> -static inline void mptcp_parse_option(const struct sk_buff *skb,
> -				      const unsigned char *ptr, int opsize,
> -				      struct tcp_options_received *opt_rx)
> -{
> -}
> -
>  static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
>  				     unsigned int *size,
>  				     struct mptcp_out_options *opts)

Nice catch!

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported

[Mat Martineau]

On Sat, 22 Jan 2022, Matthieu Baerts wrote:

> Options parsing in now done from mptcp_incoming_options().
>
> mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
> defined but not when it is not.
>
> Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> include/net/mptcp.h | 6 ------
> 1 file changed, 6 deletions(-)
>

Looks good to me, but it seems more like a net-next cleanup patch. When 
building without CONFIG_MPTCP I don't see mptcp_parse_option showing up as 
a symbol in the files that include mptcp.h.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 04f53352a1c9..8b1afd6f5cc4 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
> 	return false;
> }
>
> -static inline void mptcp_parse_option(const struct sk_buff *skb,
> -				      const unsigned char *ptr, int opsize,
> -				      struct tcp_options_received *opt_rx)
> -{
> -}
> -
> static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
> 				     unsigned int *size,
> 				     struct mptcp_out_options *opts)
> -- 
> 2.33.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Geliang Tang]

Hi Matt,

I guess we will get this error:

'''
01 signal addresses race test           syn[fail] got 2 JOIN[s] syn expected 3
 - synack[fail] got 2 JOIN[s] synack expected 3
 - ack[fail] got 2 JOIN[s] ack expected 3

                                        add[ ok ] - echo  [ok]
'''

But it didn't happen in my tests. I only got this error:

'''
01 signal addresses race test           syn[fail] got 2 JOIN[s] syn expected 3
 - synack[fail] got 2 JOIN[s] synack expected 3
 - ack[fail] got 2 JOIN[s] ack expected 3

                                        add[ ok ] - echo  [fail] got 2
ADD_ADDR echo[s] expected 3
'''

I'm not sure now. Please ignore my comments.

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月24日周一 17:39写道：
>
> Hi Geliang,
>
> Thank you for looking at that!
>
> On 22/01/2022 15:04, Geliang Tang wrote:
> > I think in this test chk_join_nr is omitted on purpose, since it is more
> > likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
> > chk_join_nr, it will fail more frequently.
>
> May you explain why this tests is likely to fail? Both the commit
> message and the comment in the code don't explain that.
>
> > Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
> >>
> >> This function also writes the name of the test with its ID, making clear
> >> a new test has been executed.
> >>
> >> Without that, the ADD_ADDR results from this test was appended at the
> >> end of the previous test causing confusions. Especially when the second
> >> test was failing, we had:
> >>
> >>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
> >>                                   add[ ok ] - echo  [ ok ]
> >>                                   add[fail] got 2 ADD_ADDR[s] expected 3
> >>
> >> In fact, this 17th test was OK but not the 18th one.
> >>
> >> Now we have:
> >>
> >>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
> >>                                   add[ ok ] - echo  [ ok ]
> >>   18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
> >>                                   add[fail] got 2 ADD_ADDR[s] expected 3
> >
> > I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
> > ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
> > passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
> > translated after ADD_ADDR, it may be lost.
>
> Indeed, to simplify the commit message, I mixed both a "success" and a
> "failure" output. I should not have done that and only put a "success"
> one to keep the output small.
>
> >> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
> >> Reported-by: Paolo Abeni <pabeni@redhat.com>
> >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> ---
> >>  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 76c45c845d3b..67be3e1215d7 100755
> >> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> @@ -1342,6 +1342,7 @@ signal_address_tests()
> >>         pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
> >>         pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
> >>         run_tests $ns1 $ns2 10.0.1.1
> >> +       chk_join_nr "signal addresses race test" 3 3 3
> >
> > I think we can omit the join numbers check, just print out the name of the
> > test. Don't pass the join numbers to chk_join_nr, just the msg:
> >
> >         chk_join_nr "signal addresses race test"
>
> Do you mean we could have situations where ADD_ADDR have been properly
> sent and received but we check counters just before MPJ are emitted? In
> this case we would mark the test as failed because MPJs are not OK
> (checked too soon in fact) while ADD_ADDR are?
>
> If yes and if it helps, we can also wait for the kernel to have sent
> exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
> already cover this UC where we look for MPJs emitted after having
> received an ADD_ADDR, no?
>
> If we keep checking all counters, I think it might be clearer: MPJ
> counters are not OK because we didn't receive/process the ADD_ADDR, no?
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]

On Mon, 24 Jan 2022, Matthieu Baerts wrote:

> Hi Geliang,
>
> Thank you for looking at that!
>
> On 22/01/2022 15:04, Geliang Tang wrote:
>> I think in this test chk_join_nr is omitted on purpose, since it is more
>> likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
>> chk_join_nr, it will fail more frequently.
>
> May you explain why this tests is likely to fail? Both the commit
> message and the comment in the code don't explain that.
>
>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>>>
>>> This function also writes the name of the test with its ID, making clear
>>> a new test has been executed.
>>>
>>> Without that, the ADD_ADDR results from this test was appended at the
>>> end of the previous test causing confusions. Especially when the second
>>> test was failing, we had:
>>>
>>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>                                   add[ ok ] - echo  [ ok ]
>>>                                   add[fail] got 2 ADD_ADDR[s] expected 3
>>>
>>> In fact, this 17th test was OK but not the 18th one.
>>>
>>> Now we have:
>>>
>>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>                                   add[ ok ] - echo  [ ok ]
>>>   18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>                                   add[fail] got 2 ADD_ADDR[s] expected 3
>>
>> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
>> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
>> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
>> translated after ADD_ADDR, it may be lost.
>
> Indeed, to simplify the commit message, I mixed both a "success" and a
> "failure" output. I should not have done that and only put a "success"
> one to keep the output small.
>

Given that Geliang withdrew his comments, and I don't have changes to 
request, I think this looks ok for the export branch:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Not sure if you want to adjust the commit message as you mention just 
above.

-Mat

>>> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
>>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> ---
>>>  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 76c45c845d3b..67be3e1215d7 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> @@ -1342,6 +1342,7 @@ signal_address_tests()
>>>         pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
>>>         pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>>>         run_tests $ns1 $ns2 10.0.1.1
>>> +       chk_join_nr "signal addresses race test" 3 3 3
>>
>> I think we can omit the join numbers check, just print out the name of the
>> test. Don't pass the join numbers to chk_join_nr, just the msg:
>>
>>         chk_join_nr "signal addresses race test"
>
> Do you mean we could have situations where ADD_ADDR have been properly
> sent and received but we check counters just before MPJ are emitted? In
> this case we would mark the test as failed because MPJs are not OK
> (checked too soon in fact) while ADD_ADDR are?
>
> If yes and if it helps, we can also wait for the kernel to have sent
> exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
> already cover this UC where we look for MPJs emitted after having
> received an ADD_ADDR, no?
>
> If we keep checking all counters, I think it might be clearer: MPJ
> counters are not OK because we didn't receive/process the ADD_ADDR, no?
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check

[Matthieu Baerts]

Hi Mat, Geliang, Paolo,

On 26/01/2022 02:26, Mat Martineau wrote:
> On Mon, 24 Jan 2022, Matthieu Baerts wrote:
> 
>> Hi Geliang,
>>
>> Thank you for looking at that!
>>
>> On 22/01/2022 15:04, Geliang Tang wrote:
>>> I think in this test chk_join_nr is omitted on purpose, since it is more
>>> likely to fail. As we can see, chk_add_nr failed sometimes, if we add
>>> this
>>> chk_join_nr, it will fail more frequently.
>>
>> May you explain why this tests is likely to fail? Both the commit
>> message and the comment in the code don't explain that.
>>
>>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六
>>> 17:32写道：
>>>>
>>>> This function also writes the name of the test with its ID, making
>>>> clear
>>>> a new test has been executed.
>>>>
>>>> Without that, the ADD_ADDR results from this test was appended at the
>>>> end of the previous test causing confusions. Especially when the second
>>>> test was failing, we had:
>>>>
>>>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>>                                   add[ ok ] - echo  [ ok ]
>>>>                                   add[fail] got 2 ADD_ADDR[s]
>>>> expected 3
>>>>
>>>> In fact, this 17th test was OK but not the 18th one.
>>>>
>>>> Now we have:
>>>>
>>>>   17 signal invalid addresses     syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>>                                   add[ ok ] - echo  [ ok ]
>>>>   18 signal addresses race test   syn[ ok ] - synack[ ok ] - ack[ ok ]
>>>>                                   add[fail] got 2 ADD_ADDR[s]
>>>> expected 3
>>>
>>> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
>>> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
>>> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
>>> translated after ADD_ADDR, it may be lost.
>>
>> Indeed, to simplify the commit message, I mixed both a "success" and a
>> "failure" output. I should not have done that and only put a "success"
>> one to keep the output small.
>>
> 
> Given that Geliang withdrew his comments, and I don't have changes to
> request, I think this looks ok for the export branch:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for the reviews and suggestions!

> Not sure if you want to adjust the commit message as you mention just
> above.

Yes, just did by writing output from the CI:

https://api.cirrus-ci.com/v1/artifact/task/5375837896704000/summary/summary.txt


- 948d50855388: mptcp: mptcp_parse_option is no longer exported
- ee286fda85da: selftests: mptcp: add missing join check
- Results: 419c18898efd..d57649f20e6e

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220128T164437
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

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