Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial coverage for RM_ADDR #52

Open
wants to merge 1 commit into
base: mptcp-net-next
Choose a base branch
from

Conversation

dcaratti
Copy link
Collaborator

Link: multipath-tcp/mptcp_net-next#167
Signed-off-by: Davide Caratti [email protected]

Copy link
Member

@matttbe matttbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for this initial support!

It looks good, I just added a couple of questions and suggestions if you don't mind looking. Up to you for some of them

+0.205 fcntl(3, F_SETFL, O_RDWR) = 0 // set back to blocking

+0.01 write(3, ..., 100) = 100
+0.0 > P. 1:101(100) ack 1 <nop, nop, TS val 305 ecr 700, mpcapable v1 flags[flag_h] key[ckey, skey] mpcdatalen 100, nop, nop>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually align the mpcapable part together. If it is OK for you to change :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will do

+0.0 > . 101:101(0) ack 1 <nop, nop, TS val 494 ecr 700, add_address addr[saddr] addr_echo>

// remove address
+0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, remove_address address_id=[1], dss dack8=101 dll=0 nocs>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we establish a new SF before and then send the RM_ADDR to see if we close the subflow? (not sure how to check that → counters?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we establish a new SF before and then send the RM_ADDR to see if we close the subflow? (not sure how to check that → counters?)

@matttbe good point: unless we make sure that the environment does not block the outbound MP_JOIN SYN (resulting from the reception of the ADD_ADDR in line 31), this line potentially races with the creation of a new subflow. Better to test this more common scenario in this case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless we make sure that the environment does not block the outbound MP_JOIN SYN (resulting from the reception of the ADD_ADDR in line 31), this line potentially races with the creation of a new subflow. Better to test this more common scenario in this case

I'm sorry, I'm not sure to understand :)
If the client is configured with only one IP and can establish new subflows once an ADD_ADDR is received, we can have a SYN+MPJ. Then we can simulate the fact that the server has lost the announced IP (or the original one) and check where data are transferred. We could also check counters or set one subflow as backup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattbe, I'm redesigning this on top of the fix for issue #143. re-testing it with a MP_JOIN subflow

+0.0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
+0.0 > S 0:0(0) <mss 1460, sackOK, TS val 100 ecr 0, nop, wscale 8, mpcapable v1 flags[flag_h] nokey>
+0.0 < S. 0:0(0) ack 1 win 65535 <mss 1460, sackOK, TS val 700 ecr 100, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey=2]>
+0.0 > . 1:1(0) ack 1 <nop, nop, TS val 100 ecr 700, mpcapable v1 flags[flag_h] key[ckey, skey]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually align all the ack and win like you did in the other pkt scripts. We can change that if you think it is not interesting to align them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will do

+0 listen(3, 1) = 0
+0 < S 0:0(0) win 32792 <mss 1460, sackOK, nop, nop, nop, wscale 7, mpcapable v1 flags[flag_h] nokey>
+0 > S. 0:0(0) ack 1 <mss 1460, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
+0.01 < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here for the ack and win alignment with the rest below

`../common/defaults.sh`

+0 `ip mptcp endpoint flush`
+0 `ip mptcp endpoint add 198.51.100.2 id 10 signal`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we call a script that checks if we are in v4/v6, could we merge both v4 and v6 script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. As long as we don't care about matching the value of the address in the ADD_ADDR, that should be do-able. Ideally, it would be nice to to "parametrize" the script so that the number of endpoints is variable: that would allow also writing a script that adds coverage for multi-id RM_ADDR (that seems to be supported by the parser, so it would deserve some coverage as well). @matttbe, WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I thought about the address but on the other hand, I don't think we check it elsewhere (add_addr). Do we?
If we do, maybe OK to have this small script dedicated to one or the others, at least we check that :-)

If we need to validate more advanced situations, we would use common/server.sh I guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we call a script that checks if we are in v4/v6, could we merge both v4 and v6 script?

with a fix similar done for issue #143 , we can test v4 and v6 using the same script.

+0 listen(3, 1) = 0
+0 < S 0:0(0) win 32792 <mss 1460, sackOK, nop, nop, nop, wscale 7, mpcapable v1 flags[flag_h] nokey>
+0 > S. 0:0(0) ack 1 <mss 1460, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
+0.01 < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here for the ack and win alignment with the rest below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will do

@matttbe
Copy link
Member

matttbe commented Sep 19, 2021

@dcaratti what should we do with this PR? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants