[MPTCP] Re: [PATCH mptcp-next v4 3/3] selftests: mptcp: add IPv4-mapped IPv6 testcases

[Matthieu Baerts <matthieu.baerts at tessares.net>]

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

Hi Geliang,

On 21/12/2020 11:16, Geliang Tang wrote:
> Hi Matt,
> 
> Matthieu Baerts <matthieu.baerts(a)tessares.net> 于2020年12月19日周六 下午9:07写道：
>>
>> From: Geliang Tang <geliangtang(a)gmail.com>
>>
>> Here, we make sure we support IPv4-mapped in IPv6 addresses in different
>> contexts:

(...)

>> +# no subflow IPv6 to v4 address
>> +reset
>> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl add dead:beef:2::10.0.3.2 flags subflow
> 
> dead:beef:2::10.0.3.2 is a invalid address, should we use dead:beef:3::2 here?

I used "dead:beef:2::10.0.3.2" on purpose just to have a valid and 
usable IPv4 address at the end of the v6 one. If we try to use the IPv4 
address at the end of this v6 one, we will send the SYN+MPJ to the 
correct host (no routing issue). With the previous version of this 
patchset (v3), this was creating a subflow with 10.0.3.2!

I should certainly add a new comment here.
Maybe you would prefer an additional test with "dead:beef:3::2" as well?

>> +run_tests $ns1 $ns2 "10.0.1.1"
> 
> We can drop the quotation marks here, only the IPv4-mapped address need
> the quotation marks.

Yes, I usually add quotes in Bash for each string but I see we didn't do 
that for the other tests, I will remove that!

> And add "0 0 0 slow" behind it, since the test need to slow down when use
> the IPv6 address.

Do I need to do that here as well even if we don't create v6 subflows?

>> +chk_join_nr "no JOIN with different families v4-v6" 0 0 0
> 
> This string is too long, it break the alignment of the output. Can we use
> "diff" here instead of "different"?

Good point, I didn't check that! I will short "different", good idea!

>> +
>> +# no subflow IPv4 to v6 address
>> +reset
>> +ip netns exec $ns1 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl limits 0 1
>> +ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
>> +run_tests $ns1 $ns2 dead:beef:1::1
> 
> Add "0 0 0 slow" here.

Same here, is it needed if we don't create any IPv6 subflow?

>> +chk_join_nr "no JOIN with different families v6-v4" 0 0 0
> 
> This string is too long too.

Will do in v5!

Thanks for the review!

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