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

net/tcp: Fix TCP keepalive time unit misuse problem #13589

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Conversation

zs39
Copy link
Contributor

@zs39 zs39 commented Sep 24, 2024

Summary

The conn->keeptimer units is decisecond,but its unit is treated as half-second in the tcp_timer & tcp_get_timeout function.
This question was asked here:
#13493

Impact

Only affects the TCP keepalive mechanism.

Testing

Local packet capture verification.

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Sep 24, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 24, 2024

[Experimental Bot, please feedback here]

This Pull Request appears to meet the basic NuttX requirements for clarity and content.

Here's a breakdown based on the provided information:

Strengths:

  • Clear Summary: The summary effectively explains the problem (unit mismatch in keepalive timer), the affected code (tcp_timer & tcp_get_timeout), and provides a link to the related issue.
  • Concise Impact: The impact section is brief but directly addresses the affected area (TCP keepalive). More details on the specific effects might be beneficial.
  • Testing Evidence: The PR includes a section for testing logs, demonstrating an effort to verify the changes.

Areas for Potential Improvement:

  • Expand on Impact: While mentioning the affected area is good, consider adding:
    • Specific consequences of the bug: What were the real-world effects of the unit mismatch?
    • How the fix addresses these: How does the PR ensure the keepalive mechanism now works as intended?
  • Detailed Testing:
    • Provide context for the logs: What do the logs represent? What tools were used to capture them?
    • Highlight key differences: Instead of just raw logs, point out the specific changes observed before and after the fix.
  • Consider Additional Impact Fields:
    • Impact on compatibility: While you state it only affects keepalive, double-check if any compatibility concerns might arise from the change.
    • Impact on documentation: If the keepalive mechanism's behavior was previously undocumented or incorrectly documented, mention the need for updates.

Overall: The PR presents the necessary information clearly. Enhancing the testing section and providing slightly more detail on the impact would further strengthen the submission.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

Please include commit message with a quick/small overview of the issue/fix.

Summary:
  The conn->keeptimer units is decisecond,but its unit is treated as
half-second in the tcp_timer & tcp_get_timeout function.
  Therefore conn>keeptimer needs to be divided by 5(DSEC_PER_HSEC)
to match half-second units.

Signed-off-by: zhangshuai39 <[email protected]>
@zs39
Copy link
Contributor Author

zs39 commented Sep 25, 2024

Please include commit message with a quick/small overview of the issue/fix.

Done

@acassis acassis merged commit 5e74ed8 into apache:master Sep 25, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HELP] TCP IP KeepAlive config parameters are not applied correctly on Nuttx 12.6 STM32H7
4 participants