-
Notifications
You must be signed in to change notification settings - Fork 2.1k
gnrc_tcp: rewrite passive open #16459
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
gnrc_tcp: rewrite passive open #16459
Conversation
Should we move this to next release as it is soft feature freeze and this contains an API change? |
This PR and the followup PR contain large API changes. Although the changes are tested in depth, it might be much effort to review all of this. I personally don't care if this is part of this release or the next. From my side, do whatever relaxes your release management. ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good at a cursory glance
7dc3013
to
e74e8b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, Murdock should ensure that the tests are still passing on native.
Given that the tests require root it probably doesn't ;-). |
True - but Murdock found that |
Should be fixed by now |
Nevertheless the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I looked at the code and found a problem 😅 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, why my review comment did not get posted though.
Mh test works on |
Here it is
|
The log seems strange to me since it sends gnrc_tcp_open_passive to the target, a function that should not exist anymore. Could you perform a clean build and test again? |
I don't see how a clean build has any effect on the test script ;-). |
But did just that and same result. |
Ah wait, the output indeed differs
|
Seems like a timeout thats large enough on native. I'll see if I can find a board here and try to determine a suitable value |
@miri64 - I tested the PR on the nucleo-f401re and it worked. I added a commit increasing the accept timeout in the failing test script from 2000ms to 4000ms, I hope this is long enough for all boards with less processing power. The failing test is the one where 1000 malformed packets are send and discarded, afterwards a valid packet is sent and used to accept the connection. In that test processing power might be the limiting factor. Could you test again? |
Nope. But 10000 works. How about, just to be on the save side to set it to 15000? Also if that is done, please squash the commits reflecting the review history. |
5e426bf
to
0d5db28
Compare
Done |
0d5db28 still needs to be squashed in the original commits. |
0d5db28
to
1fd9af1
Compare
Forgot this one. I squashed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Not sure why murdock is marked as failed or still doing label checks... |
@miri64: Should I squash it in a single commit? I splitted the PR only to help with the review. EDIT: Since all is reviewed and green, I just squash the entire thing and merge it. |
1fd9af1
to
88a0273
Compare
Maybe my mind is playing a trick on me but I could swere that there was I time were I was able to press the merge button for my own PR after a successfull review. But I had never maintainer status. |
Contribution description
This PR rewrites the passive connection establishment mechanism of GNRC_TCP.
The new version aligns the GNRC_TCP interface with the SOCK_TCP interface with the goal to make a upcoming SOCK integration straight forward and trivial.
Since this is a large change, I've split the PR into three commits (api, implementation and tests) to help reviewing this.
The new tests are written in different style than existing tests, please ignore the difference in style for now, I plan to align the existing test structure in a dedicated PR on after merging this.
Testing procedure
This PR is covered by the GNRC_TCP test suite.
To test this PR follow the instructions under tests/gnrc_tcp/Readme.md
Issues/PRs references
Fixes #10664 - TCP Sockets can not be used / built