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

Support TCP Timestamp option #939

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

tomDev5
Copy link
Contributor

@tomDev5 tomDev5 commented Jun 1, 2024

This is currently not relating to the RTT estimation, just for supporting remotes who do support it.
The generator isn't a lambda to avoid a generic.
Tests include a logic test of an established session and tests which make sure this option is only enabled if the both sides support it.

@whitequark
Copy link
Contributor

Looks good at a quick glance.

@tomDev5
Copy link
Contributor Author

tomDev5 commented Jun 14, 2024

Hi, I would appreciate if someone could review this PR (or approve if it indeed looks good)

src/socket/tcp.rs Outdated Show resolved Hide resolved
src/wire/tcp.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

I would like you to also add RFC 7323 to the src/socket/tcp.rs file header, since it is also now necessary to understand the semantics. Once you're done please squash the commits.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 93.36493% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.21%. Comparing base (6c06cd9) to head (b4c0779).
Report is 31 commits behind head on main.

Current head b4c0779 differs from pull request most recent head 89e8265

Please upload reports for the commit 89e8265 to get more accurate results.

Files Patch % Lines
src/wire/tcp.rs 73.58% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
- Coverage   79.97%   79.21%   -0.77%     
==========================================
  Files          81       81              
  Lines       27831    26820    -1011     
==========================================
- Hits        22259    21245    -1014     
- Misses       5572     5575       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomDev5 tomDev5 force-pushed the tcp_timestamp branch 2 times, most recently from 4d4824e to ccd8c7f Compare June 14, 2024 11:36
@tomDev5
Copy link
Contributor Author

tomDev5 commented Jun 14, 2024

Ok, the codecov window is refusing to update.
all complained upon lines are integrated in tests

@tomDev5 tomDev5 requested a review from whitequark June 14, 2024 12:11
@whitequark
Copy link
Contributor

Ok, the codecov window is refusing to update.

Yeah so codecov is having some infra issues. That's ok. I talked to their CTO, they'll fix it eventually but not yet.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

This is a lot better but I'd still much prefer there to be a struct for the timestamp itself with inherent method(s) to manipulate it to the current situation with extensive tuple access. I can take a closer look and propose a specific structure if you'd like.

@tomDev5
Copy link
Contributor Author

tomDev5 commented Jun 14, 2024

Agreed, that made for cleaner access.

@tomDev5 tomDev5 requested a review from whitequark June 14, 2024 13:01
Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Sorry, I was unclear, see inline comments.

src/wire/tcp.rs Outdated Show resolved Hide resolved
src/socket/tcp.rs Outdated Show resolved Hide resolved
src/socket/tcp.rs Outdated Show resolved Hide resolved
@tomDev5 tomDev5 requested a review from whitequark June 14, 2024 13:32
@whitequark
Copy link
Contributor

I'm on vacation for a week. Later!

@tomDev5
Copy link
Contributor Author

tomDev5 commented Jun 25, 2024

@whitequark gentle ping, regarding the fixes I've made

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the work you put into this and the fixes.

@whitequark whitequark added this pull request to the merge queue Jun 26, 2024
Merged via the queue into smoltcp-rs:main with commit 23697fa Jun 26, 2024
9 checks passed
@tomDev5 tomDev5 deleted the tcp_timestamp branch June 26, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants