-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: port 0 is not a bad one #6544
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
Conversation
6535 follow-up
dropped for merge commit simplifification |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/netbase_tests.cpp (1)
Line range hint
454-465
: Consider making the bad ports count test more maintainable.While the count verification is good practice, consider defining the expected count as a named constant. This would make the test more maintainable when the bad port list changes in the future.
+ static constexpr size_t EXPECTED_BAD_PORTS = 21; size_t total_bad_ports{0}; for (uint16_t port = std::numeric_limits<uint16_t>::max(); port > 0; --port) { if (IsBadPort(port)) { ++total_bad_ports; } } - BOOST_CHECK_EQUAL(total_bad_ports - PRIVILEGED_PORTS_THRESHOLD, 21); + BOOST_CHECK_EQUAL(total_bad_ports - PRIVILEGED_PORTS_THRESHOLD, EXPECTED_BAD_PORTS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/netbase.cpp
(1 hunks)src/test/netbase_tests.cpp
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (2)
src/netbase.cpp (1)
712-712
: LGTM! The change correctly allows port 0 for ephemeral port allocation.The modification to exclude port 0 from bad ports is correct. In POSIX socket programming, binding to port 0 is a special case that tells the operating system to allocate an ephemeral port, which is a common and valid practice.
src/test/netbase_tests.cpp (1)
453-453
: LGTM! Comprehensive test coverage for the port 0 change.The test case properly verifies that port 0 is now allowed while maintaining checks for other port classifications.
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.
utACK 8c30f73
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.
utACK 8c30f73; nit, could've been one commit
Issue being fixed or feature implemented
source
#6535 follow-up
What was done?
How Has This Been Tested?
Breaking Changes
Checklist: