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.

Sign up for GitHub

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

Jump to bottom

Fixing port selection in TCP UTs #2739

Merged
merged 5 commits into from
May 23, 2024
Merged

Fixing port selection in TCP UTs #2739

merged 5 commits into from
May 23, 2024

Conversation

LeStarch
Copy link
Collaborator

@LeStarch LeStarch commented May 21, 2024 •
edited
Loading

Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)

Change Description

Fixes the TCP port selection problem for TCP UTs. This was done by:

  1. Allowing Tcp and UDP recv to use port "0" (have OS assign port).
  2. Add read-back for "0" ports.
  3. Supply read-back port to UT as the connection port
  4. Correcting port selection and readback for network byte order

github-advanced-security[bot]
github-advanced-security bot found potential problems May 21, 2024
View reviewed changes
Drv/Ip/TcpClientSocket.hpp Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems May 21, 2024
View reviewed changes
Drv/Ip/TcpServerSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/UdpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/IpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/IpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/IpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/TcpClientSocket.cpp Dismissed Show dismissed Hide dismissed
github-advanced-security[bot]
github-advanced-security bot found potential problems May 21, 2024
View reviewed changes
Drv/Ip/TcpServerSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/UdpSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/TcpServerSocket.cpp Dismissed Show dismissed Hide dismissed
Drv/Ip/TcpClientSocket.cpp Dismissed Show dismissed Hide dismissed
@LeStarch LeStarch mentioned this pull request May 21, 2024
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems May 21, 2024
View reviewed changes
Drv/Ip/UdpSocket.cpp Dismissed Show dismissed Hide dismissed
@LeStarch
Copy link
Collaborator Author

Note: I have run the UTs multiple 100's of times each in a known-failing environment. No issues were revealed.

JohanBertrand
JohanBertrand reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@JohanBertrand JohanBertrand left a comment

Choose a reason for hiding this comment

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

Just a few comments, thanks for this investigation and those changes!

Drv/Ip/IpSocket.cpp Show resolved Hide resolved
Drv/Ip/test/ut/TestTcp.cpp Outdated Show resolved Hide resolved
Drv/Ip/test/ut/PortSelector.cpp Show resolved Hide resolved
Drv/Ip/test/ut/TestUdp.cpp Outdated Show resolved Hide resolved
@LeStarch
Copy link
Collaborator Author

@JohanBertrand I should have fixed both issues!

@JohanBertrand
Copy link
Contributor

All good from my side, thank you!

kevin-f-ortega
kevin-f-ortega approved these changes May 23, 2024
View reviewed changes
Copy link
Collaborator

@kevin-f-ortega kevin-f-ortega left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work! We can finally put this random UT failure to rest.

@kevin-f-ortega kevin-f-ortega merged commit e55ebca into nasa:devel May 23, 2024
35 checks passed
@JohanBertrand JohanBertrand mentioned this pull request May 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohanBertrand JohanBertrand JohanBertrand left review comments

@kevin-f-ortega kevin-f-ortega kevin-f-ortega approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeStarch @JohanBertrand @kevin-f-ortega