issue: HPCINFRA-3949 distinguish remote addresses #497
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest network address configuration to support containerized test environments that lack default gateways. The key change splits the single remote_addr into two distinct fields: remote_addr (non-routable dummy addresses like 74.190.183.38 for bind failure tests) and remote_routable_addr (dynamically detected gateway addresses for connect tests). Gateway detection is moved from C++ runtime code (sys_gateway() function in common/sys.cc) to shell-script-level logic (do_get_gateway() bash function in gtest.sh), which detects the default gateway at test invocation time and passes it via a new -g/--remote-routable command-line option. This architectural shift removes the need for SKIP_TRUE(def_gw_exists) checks inside test cases, making tests more portable across bare-metal and containerized environments by explicitly controlling which address type each test receives upfront.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/common/sys.h | 5/5 | Removes sys_gateway() function declaration as part of moving gateway detection to shell layer |
| tests/gtest/common/sys.cc | 2/5 | Deletes 39-line sys_gateway() function; exposes pre-existing critical bug in sys_hexdump() with inverted null-check |
| tests/gtest/common/base.h | 5/5 | Adds remote_routable_addr field and removes def_gw_exists flag from test_base class |
| tests/gtest/common/def.h | 5/5 | Adds remote_routable_addr field to gtest_configure_t structure |
| tests/gtest/tcp/tcp_event.cc | 5/5 | Replaces remote_addr with remote_routable_addr in ti_2 and DISABLED_ti_4 tests; removes gateway skip checks |
| tests/gtest/udp/udp_connect.cc | 4/5 | Changes ti_2 to use remote_routable_addr; removes SKIP_TRUE checks; tests will fail explicitly if gateway not provided |
| tests/gtest/common/base.cc | 5/5 | Adds initialization logic for remote_routable_addr mirroring existing address-field patterns |
| tests/gtest/tcp/tcp_bind.cc | 5/5 | Removes gateway-existence skip check; test now unconditionally expects EADDRNOTAVAIL |
| tests/gtest/udp/udp_bind.cc | 4/5 | Removes gateway skip logic; assumes remote_addr will always be non-routable from shell script |
| tests/gtest/main.cc | 3/5 | Adds -g option for routable address; potential bug at lines 208-210 if only one address is user-defined |
| contrib/jenkins_tests/gtest.sh | 2/5 | Implements do_get_gateway() bash function; critical issues with empty string when no gateway found and overly restrictive grep pattern |
Confidence score: 2/5
- This PR requires careful review due to critical bugs in gateway detection and argument handling that will cause test failures in environments without gateways.
- Score lowered due to: (1)
gtest.shpassing empty string after-gwhen no gateway exists, likely causing gtest binary argument parsing errors; (2) potential partial initialization bug inmain.cclines 208-210 where one address may remain uninitialized if the other is user-defined; (3) pre-existing inverted null-check insys_hexdump()(not introduced by this PR but now visible); (4) restrictive grep pattern 'UG[ \t]' in gateway detection may miss valid gateways with additional flags. - Pay close attention to
contrib/jenkins_tests/gtest.sh(gateway detection needs defensive error handling),tests/gtest/main.cc(lines 208-210 conditional logic), andtests/gtest/common/sys.cc(lines 24-26 contain critical pre-existing bug in sys_hexdump).
Sequence Diagram
sequenceDiagram
participant Script as gtest.sh
participant Gateway as do_get_gateway()
participant Main as main.cc
participant Config as gtest_configure_t
participant Base as test_base
participant Test as Test Cases
Script->>Gateway: "Detect IPv4 gateway"
Gateway-->>Script: "gateway_ipv4 (or empty)"
Script->>Gateway: "Detect IPv6 gateway"
Gateway-->>Script: "gateway_ipv6 (or empty)"
Script->>Main: "Launch gtest with args:<br/>-r (non-routable)<br/>-g (routable gateway)"
Main->>Main: "Parse command line options"
Main->>Config: "Set remote_addr (e.g., 74.190.183.38)"
Main->>Config: "Set remote_routable_addr (detected gateway)"
Main->>Config: "Set client_addr, server_addr, port"
Main->>Base: "Initialize test_base()"
Base->>Config: "Read gtest_conf"
Base->>Base: "Copy remote_addr to member"
Base->>Base: "Copy remote_routable_addr to member"
Base->>Base: "Convert IPv4 to IPv6 mapped if needed"
Main->>Test: "Run test cases"
alt Bind failure test (tcp_bind.ti_2, udp_bind.ti_2)
Test->>Test: "Use remote_addr (non-routable)"
Test->>Test: "Expect EADDRNOTAVAIL error"
else Connect test (tcp_event.ti_2, udp_connect.ti_2)
Test->>Test: "Use remote_routable_addr (gateway)"
Test->>Test: "Attempt actual routing"
end
11 files reviewed, 4 comments
There was a problem hiding this comment.
Greptile Overview
{"
Sequence Diagram
sequenceDiagram
participant User
participant gtest.sh
participant do_get_gateway
participant gtest_app
participant test_base
participant tcp_bind/udp_bind
participant tcp_event/udp_connect
User->>gtest.sh: Run gtest tests
gtest.sh->>do_get_gateway: Detect IPv4 gateway
do_get_gateway->>gtest.sh: Return gateway_ipv4
gtest.sh->>do_get_gateway: Detect IPv6 gateway
do_get_gateway->>gtest.sh: Return gateway_ipv6
gtest.sh->>gtest.sh: Build gtest_opt with -r (non-routable) and -g (routable)
gtest.sh->>gtest_app: Execute with --addr, -r 74.190.183.38, -g gateway_ipv4
gtest_app->>gtest_app: Parse command-line options (-r, -g)
gtest_app->>gtest_app: Set gtest_conf.remote_addr (non-routable)
gtest_app->>gtest_app: Set gtest_conf.remote_routable_addr (routable)
gtest_app->>test_base: Initialize test with addresses
test_base->>test_base: Copy remote_addr to member variable
test_base->>test_base: Copy remote_routable_addr to member variable
test_base->>test_base: Convert IPv4 to IPv6 if needed
alt Bind failure tests
test_base->>tcp_bind: ti_2 test
tcp_bind->>tcp_bind: bind() to remote_addr (non-routable)
tcp_bind->>tcp_bind: Expect EADDRNOTAVAIL error
test_base->>udp_bind: ti_2 test
udp_bind->>udp_bind: bind() to remote_addr (non-routable)
udp_bind->>udp_bind: Expect EADDRNOTAVAIL error
end
alt Connect/routing tests
test_base->>tcp_event: ti_2 test
tcp_event->>tcp_event: connect() to remote_routable_addr (gateway)
tcp_event->>tcp_event: Expect EINPROGRESS then EPOLLERR|EPOLLHUP
test_base->>udp_connect: ti_2 test
udp_connect->>udp_connect: connect() to remote_routable_addr (gateway)
udp_connect->>udp_connect: Expect success (errno=EOK)
end
gtest_app->>User: Return test results
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)logic:
if (ptr) return;will skip the entire dump when pointer is valid. This should beif (!ptr) return;- the logic is inverted, causing the function to exit early when it should actually perform the dump
11 files reviewed, 3 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest infrastructure to distinguish between two types of remote addresses: remote_addr (non-routable, for bind failure tests) and remote_routable_addr (routable, for connect tests). It moves gateway detection from C++ runtime code (sys_gateway()) to the shell script layer (do_get_gateway() in gtest.sh), addressing container environments where spawned test containers lack default gateway configuration. The def_gw_exists flag is removed from test infrastructure; instead, the shell script detects gateways before test invocation and passes appropriate addresses via new -r and -g command-line flags. This architectural shift decouples environment discovery from test logic: tests expecting routable addresses receive actual gateway IPs, while bind failure tests continue using hardcoded non-routable dummy addresses (74.190.183.38 for IPv4, fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff for IPv6). The change touches test fixtures, configuration structures, command-line parsing, and several TCP/UDP test cases that previously used runtime skip logic.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/common/sys.cc | 1/5 | CRITICAL: Introduced inverted logic bug in sys_hexdump() line 24; condition should be if (!ptr) not if (ptr) |
| contrib/jenkins_tests/gtest.sh | 3/5 | Adds do_get_gateway() bash function; missing input validation for empty gateway strings in -g flag |
| tests/gtest/main.cc | 3/5 | Adds -g option and dual-address logic; defaults may leave addresses uninitialized if only one flag is passed |
| tests/gtest/common/base.cc | 4/5 | Initializes remote_routable_addr with IPv4-to-IPv6 conversion; clean implementation following existing pattern |
| tests/gtest/common/def.h | 4/5 | Adds remote_routable_addr field and removes def_gw_exists; structural change enabling dual-address strategy |
| tests/gtest/common/base.h | 4/5 | Removes def_gw_exists flag; adds remote_routable_addr member to test base class |
| tests/gtest/tcp/tcp_event.cc | 4/5 | Updates ti_2 and DISABLED_ti_4 to use remote_routable_addr; removes skip guard |
| tests/gtest/udp/udp_connect.cc | 4/5 | Changes ti_2 and ti_3 to use remote_routable_addr; removes SKIP_TRUE checks |
| tests/gtest/tcp/tcp_bind.cc | 4/5 | Removes def_gw_exists skip from ti_2 bind failure test; relies on shell-provided non-routable address |
| tests/gtest/udp/udp_bind.cc | 4/5 | Removes def_gw_exists skip from ti_2 bind failure test; expects remote_addr from shell script |
| tests/gtest/common/sys.h | 5/5 | Cleanly removes sys_gateway() declaration consistent with implementation removal |
Confidence score: 1/5
- This PR introduces critical bugs that will cause immediate failures and must be fixed before merge
- Score reflects three blocking issues: (1) inverted null-check in
sys_hexdump()breaking all hexdump functionality, (2) missing validation for empty gateway strings that will cause argument parsing errors when-greceives empty value, and (3) conditional logic inmain.ccline 208 that may leave one address uninitialized when only-ror-gis specified - Pay immediate attention to
tests/gtest/common/sys.cclines 24-26,contrib/jenkins_tests/gtest.shlines 86 and 94 (empty string handling), andtests/gtest/main.ccline 208 (default-setting condition)
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)logic: condition inverted - should be
if (!ptr)to skip dump when ptr is NULL. Currently returns early when ptr is valid, preventing any hexdump output
11 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors gtest address configuration to support containerized test execution by splitting remote addresses into two distinct concepts: remote_addr (non-routable dummy IPs for bind failure tests like tcp_bind.ti_2 and udp_bind.ti_2) and remote_routable_addr (actual gateway addresses for connect/routing tests like tcp_event.ti_2 and udp_connect.ti_2). Gateway detection moves from C++ runtime logic (sys_gateway() function) to the shell script layer (do_get_gateway() in gtest.sh), which detects the system default gateway at test invocation time via the route command and passes it using a new -g/--remote-routable command-line option. The def_gw_exists flag and associated skip checks are removed from all test code since address availability is now handled by the orchestration layer. The change integrates with the existing gtest framework by extending the gtest_configure_t structure and test option parsing in main.cc, while base.cc initializes the new address field using the same IPv4/IPv6 conversion pattern as existing address fields.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/common/sys.cc | 1/5 | Removes gateway detection function but introduces critical inverted NULL-check logic in sys_hexdump() on line 24 causing segfaults |
| contrib/jenkins_tests/gtest.sh | 2/5 | Adds gateway detection logic but contains fatal syntax error on line 92 and risks passing empty -g arguments when IPv6 gateway is unavailable |
| tests/gtest/main.cc | 3/5 | Introduces -g option and unconditionally sets port defaults but may still process empty gateway strings from shell script |
| tests/gtest/udp/udp_bind.cc | 4/5 | Removes skip check for bind failure test; correct if shell script provides proper non-routable -r address |
| tests/gtest/udp/udp_connect.cc | 4/5 | Updates connect test to use routable address; stil uses sizeof(remote_routable_addr) instead of actual address family size |
| tests/gtest/tcp/tcp_event.cc | 4/5 | Changes connect calls to use routable address and removes skip logic; clean refactor aligning with PR goals |
| tests/gtest/common/def.h | 4/5 | Adds remote_routable_addr field and removes def_gw_exists flag; structural change supporting address separation |
| tests/gtest/common/base.cc | 5/5 | Initializes new routable address field with consistent IPv4/IPv6 conversion pattern matching existing address handling |
| tests/gtest/common/base.h | 5/5 | Declares remote_routable_addr in test_base class and removes obsolete def_gw_exists flag |
| tests/gtest/common/sys.h | 5/5 | Removes sys_gateway() declaration as gateway detection moves to shell layer |
| tests/gtest/tcp/tcp_bind.cc | 0/5 | No critical sections or details provided in changed files context |
Confidence score: 1/5
- This PR is not safe to merge and will immediately break test execution due to critical bugs
- Score reflects three show-stopping issues: (1) inverted NULL-check in
sys_hexdump()causing segfaults when valid pointers are passed, (2) syntax error on line92ofgtest.shthat will prevent script execution, and (3) empty string handling for-gargument when IPv6 gateway is unavailable may cause argument parsing errors - Pay immediate attention to
tests/gtest/common/sys.ccline 24 (fixif (ptr)toif (!ptr)),contrib/jenkins_tests/gtest.shline 92 (fix syntax error in string construction), and the gateway parameter logic on lines 80-93 ofgtest.sh(handle empty IPv6 gateway case properly)
Sequence Diagram
sequenceDiagram
participant User
participant gtest.sh
participant route_cmd as route command
participant gtest_app as gtest application
participant main.cc
participant test_base
participant tests as Test Cases
User->>gtest.sh: "Execute test script"
gtest.sh->>route_cmd: "do_get_gateway('inet')"
route_cmd-->>gtest.sh: "IPv4 gateway address"
gtest.sh->>route_cmd: "do_get_gateway('inet6')"
route_cmd-->>gtest.sh: "IPv6 gateway address (optional)"
gtest.sh->>gtest.sh: "Build command with -r (non-routable) and -g (routable) addresses"
gtest.sh->>gtest_app: "env LD_PRELOAD=... gtest --addr=... -r 74.190.183.38 -g <gateway>"
gtest_app->>main.cc: "Parse command line arguments"
main.cc->>main.cc: "Process -r and -g options"
main.cc->>main.cc: "Store in gtest_conf.remote_addr and gtest_conf.remote_routable_addr"
main.cc->>test_base: "Initialize test base classes"
test_base->>test_base: "Copy remote_addr and remote_routable_addr from gtest_conf"
main.cc->>tests: "RUN_ALL_TESTS()"
tests->>tests: "tcp_bind.ti_2: bind to remote_addr (non-routable)"
tests->>tests: "tcp_event.ti_2: connect to remote_routable_addr (gateway)"
tests->>tests: "udp_bind.ti_2: bind to remote_addr (non-routable)"
tests->>tests: "udp_connect.ti_2: connect to remote_routable_addr (gateway)"
tests-->>gtest.sh: "Test results and exit code"
gtest.sh-->>User: "Test execution complete"
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)logic: inverted condition in hexdump. Returns early when
ptris valid, causing the function to never execute. Should beif (!ptr)to match original behavior and log message on line 27
11 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest network address configuration to separate non-routable addresses (used in bind failure tests) from routable addresses (used in connect/routing tests). It removes runtime gateway detection logic (sys_gateway() in C++) and the def_gw_exists flag, moving gateway discovery into the shell orchestration layer (gtest.sh). The shell script now detects default gateways at test invocation time and passes two distinct addresses via command-line arguments: -r for a hardcoded non-routable dummy address (e.g., 74.190.183.38) and -g for the dynamically detected gateway. This architectural change makes the test suite portable to containerized CI environments that lack default gateways, as the shell can conditionally skip tests requiring routing when no gateway exists, rather than each test performing runtime checks and skipping itself. The codebase's existing test framework already supports multiple parameterized addresses (client_addr, server_addr, remote_addr), so adding remote_routable_addr follows established patterns.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/common/sys.cc | 1/5 | Removed sys_gateway() function but introduced inverted null-check bug in sys_hexdump() (line 24) that prevents function from ever executing |
| contrib/jenkins_tests/gtest.sh | 3/5 | Added do_get_gateway() bash function for gateway detection; has regex issue matching compound route flags and potential empty string handling problems |
| tests/gtest/main.cc | 3.5/5 | Added -g flag and removed gateway detection; conditional logic for default address initialization may leave addresses uninitialized when only one flag is provided |
| tests/gtest/common/base.cc | 4/5 | Added initialization for remote_routable_addr following existing address conversion pattern; clean implementation with minor style concerns about address size |
| tests/gtest/tcp/tcp_event.cc | 4/5 | Updated to use remote_routable_addr; passes full sockaddr_storage size to connect() which may send padding bytes for IPv4 |
| tests/gtest/udp/udp_connect.cc | 4/5 | Changed to remote_routable_addr and removed skip logic; depends on shell script correctly populating the routable address |
| tests/gtest/udp/udp_bind.cc | 4/5 | Removed gateway skip check; test now runs unconditionally with non-routable address from -r flag |
| tests/gtest/common/def.h | 5/5 | Added remote_routable_addr field and removed def_gw_exists flag; clean structural change aligned with refactoring goals |
| tests/gtest/common/base.h | 5/5 | Added remote_routable_addr member and removed def_gw_exists flag; straightforward header update with no functional logic |
| tests/gtest/tcp/tcp_bind.cc | 5/5 | Removed unnecessary gateway skip check; test logic unchanged and appropriate for non-routable address validation |
| tests/gtest/common/sys.h | 5/5 | Removed sys_gateway() declaration as part of moving detection to shell layer; clean deletion with no side effects |
Confidence score: 2/5
- This PR introduces a critical bug and multiple edge-case handling issues that will cause test failures or incorrect behavior in production CI environments
- Score reflects a blocking bug in sys_hexdump() (inverted null check), fragile gateway detection regex that will miss compound route flags, empty string handling issues in gtest.sh that will produce malformed command-line arguments, and incomplete default address initialization logic in main.cc that can leave addresses zeroed when only one flag is provided
- Pay extremely close attention to tests/gtest/common/sys.cc (line 24 null check must be fixed), contrib/jenkins_tests/gtest.sh (lines 70-71, 73-74 regex patterns and lines 86, 92 empty string handling), and tests/gtest/main.cc (lines 206-208 conditional logic for set_def_remote_address)
Sequence Diagram
sequenceDiagram
participant User
participant gtest.sh
participant do_get_gateway
participant gtest_binary
participant main.cc
participant test_base
participant tcp_event/udp_connect
11 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest framework to distinguish between routable and non-routable remote addresses, enabling tests to run in containerized environments without default gateways. Previously, tests used a single remote_addr and runtime gateway detection (sys_gateway() in C++, def_gw_exists flag) to conditionally skip routing tests. The new design introduces two separate addresses: remote_addr (non-routable, e.g., 74.190.183.38) for bind-failure tests, and remote_routable_addr (dynamically detected gateway or user-specified) for connect/routing tests. Gateway detection is moved from C++ test execution to the shell script layer (gtest.sh), which invokes do_get_gateway() at test invocation time and passes the appropriate addresses via -r and -g CLI flags. This architectural shift removes the def_gw_exists flag, eliminates SKIP_TRUE() checks in individual tests, and makes the test suite portable across diverse network environments by externalizing environment detection to the orchestration layer.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| tests/gtest/common/sys.cc | 1/5 | Critical NULL pointer logic bug in sys_hexdump (inverted NULL check causes segfault when ptr is NULL); removed sys_gateway function |
| tests/gtest/main.cc | 3/5 | Added -g CLI option for routable address; IPv4 ports may not be properly initialized when user specifies address without port |
| contrib/jenkins_tests/gtest.sh | 4/5 | Implements bash-based gateway detection with do_get_gateway; IPv4 gateway strictly required (exit 1 if missing), IPv6 optional; potential empty-string handling issues |
| tests/gtest/common/base.cc | 4/5 | Initializes new remote_routable_addr field with IPv4-to-IPv6 conversion; removed def_gw_exists flag; inherits sizeof/port concerns from similar address blocks |
| tests/gtest/common/def.h | 4/5 | Added remote_routable_addr field to gtest_configure_t; removed def_gw_exists flag; structural change shifts gateway detection responsibility to shell layer |
| tests/gtest/tcp/tcp_event.cc | 4/5 | Replaced remote_addr with remote_routable_addr in connect tests; removed SKIP_TRUE checks; uses sizeof(remote_routable_addr) which may send padding for IPv4 |
| tests/gtest/udp/udp_connect.cc | 4/5 | Updated ti_2 to use remote_routable_addr; removed gateway skip checks; sizeof usage may send extra bytes for IPv4 addresses |
| tests/gtest/common/base.h | 3/5 | Added remote_routable_addr field; removed def_gw_exists flag; tests now rely on shell-provided addresses instead of runtime gateway detection |
| tests/gtest/tcp/tcp_bind.cc | 5/5 | Removed SKIP_TRUE check from ti_2; test now unconditionally expects EADDRNOTAVAIL for non-routable address bind |
| tests/gtest/udp/udp_bind.cc | 5/5 | Removed SKIP_TRUE check from ti_2; test logic unchanged, relies on shell script to provide non-routable address via -r |
| tests/gtest/common/sys.h | 5/5 | Removed sys_gateway function declaration; clean header change with no implementation issues |
Confidence score: 1/5
- This PR introduces a critical NULL pointer dereference bug in sys_hexdump that will cause immediate segfaults when the function is called with a NULL pointer
- Score reflects the inverted NULL check in tests/gtest/common/sys.cc (lines 24-26) where the condition
if (ptr) { return; }causes the function to exit when ptr is valid and continue to dereference ptr when it is NULL, guaranteeing a crash - The sys.cc file requires immediate attention to fix the NULL check logic; additional concerns include potential IPv4 port initialization issues in main.cc when users provide addresses without ports, and empty gateway string handling in gtest.sh that could cause argument parsing errors
Sequence Diagram
sequenceDiagram
participant User
participant gtest.sh as "gtest.sh (Shell Script)"
participant route as "route command"
participant gtest as "gtest Application"
participant test_base as "test_base (C++)"
participant tests as "Test Cases"
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)syntax: inverted condition - should be
if (!ptr)to return early when ptr is NULL. Currently returns when ptr is valid, causing the rest of the function to dereference NULL
11 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest infrastructure to separate concerns between non-routable and routable remote addresses, enabling tests to run in containerized environments that lack default gateways. Previously, tests relied on a single remote_addr and runtime gateway detection via the C++ sys_gateway() function, conditionally skipping tests when no gateway existed using a def_gw_exists flag. The new design introduces remote_routable_addr as a distinct field and moves gateway detection from C++ test code to the shell script layer (gtest.sh), which now invokes do_get_gateway() to detect IPv4/IPv6 gateways and passes them via the new -g/--remote-routable command-line flag. Bind-failure tests (tcp_bind.ti_2, udp_bind.ti_2) continue using the non-routable remote_addr (supplied via -r), while connect tests (tcp_event.ti_2, udp_connect.ti_2) now use remote_routable_addr. This architectural shift decouples test logic from environment discovery, making the suite more portable and eliminating the need for runtime skip checks in individual tests.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
tests/gtest/common/sys.cc |
2/5 | Removed sys_gateway() function but introduced a critical bug: inverted null-check in sys_hexdump() (if (ptr) should be if (!ptr)) causes the function to return early on valid pointers instead of null pointers |
contrib/jenkins_tests/gtest.sh |
3/5 | Added do_get_gateway() bash function for dynamic gateway detection; hardcoded non-routable dummy address 74.190.183.38may be routable in some test networks; hard exit on missing IPv4 gateway differs from permissive IPv6 handling |
tests/gtest/main.cc |
3/5 | Added -g/--remote-routable flag and split address initialization; set_def_remote_address() now runs unconditionally which may overwrite ports unexpectedly; default remote_routable_addr is localhost which will break routing tests if gtest.sh doesn't pass -g |
tests/gtest/udp/udp_connect.cc |
3/5 | Switched ti_2 to use remote_routable_addr and removed skip checks; sizeof(remote_routable_addr) passes full sockaddr_storage size instead of address-family-specific size; risk of reading uninitialized memory if -g not provided |
tests/gtest/common/base.cc |
4/5 | Introduced convert_and_copy_address() helper to eliminate duplication and added remote_routable_addr initialization; no explicit validation that remote_routable_addr is initialized before use in tests |
tests/gtest/tcp/tcp_event.cc |
4.5/5 | Updated two tests to use remote_routable_addr for connect operations; removed SKIP_TRUE(def_gw_exists) guard; minor style inconsistency with sizeof(remote_routable_addr) usage |
tests/gtest/common/base.h |
5/5 | Added remote_routable_addr field and removed def_gw_exists boolean flag; clean header change with no syntax issues |
tests/gtest/common/sys.h |
5/5 | Removed sys_gateway() function declaration cleanly as part of moving gateway detection to shell script layer |
tests/gtest/common/def.h |
5/5 | Added remote_routable_addr field to gtest_configure_t structure and removed def_gw_exists flag; aligns with PR goal of separating address types |
tests/gtest/udp/udp_bind.cc |
5/5 | Removed gateway-detection skip logic from ti_2 test; now unconditionally tests bind failure on non-routable address |
tests/gtest/tcp/tcp_bind.cc |
5/5 | No changes requiring review (likely just removed skip logic similar to udp_bind.cc) |
Confidence score: 2/5
- This PR has significant risk due to a critical bug in
sys.cc(inverted null-check insys_hexdump()), multiple uninitialized-memory risks if shell script doesn't provide-g, and behavioral changes in address initialization that may cause unexpected test failures - Score lowered primarily due to: (1) inverted null-check bug that will break hex dump functionality immediately; (2) lack of initialization validation for
remote_routable_addrcreates undefined behavior when-gis not provided; (3) hardcoded dummy address in gtest.sh may be routable in some networks; (4) unconditional call toset_def_remote_address()changes port-setting behavior; (5)sizeof(sockaddr_storage)passed to connect() instead of address-family-specific size - Pay close attention to
tests/gtest/common/sys.cc(fix the inverted null-check on line 24),tests/gtest/main.cc(ensureremote_routable_addris always initialized), andcontrib/jenkins_tests/gtest.sh(validate gateway detection logic and address selection)
Sequence Diagram
sequenceDiagram
participant Shell as gtest.sh
participant Gateway as do_get_gateway()
participant GTest as gtest binary
participant Main as main.cc
participant TestBase as test_base
participant Test as tcp_event/udp_connect
Shell->>Shell: Check dependencies (install_dir, ibdev2netdev)
Shell->>Shell: Get IP addresses via do_get_addrs()
Shell->>Gateway: do_get_gateway('inet')
Gateway->>Gateway: Execute route -n command
Gateway->>Gateway: Extract gateway with grep/awk
Gateway-->>Shell: Return IPv4 gateway address
alt No IPv4 gateway found
Shell->>Shell: Exit with error
else Gateway found
Shell->>Gateway: do_get_gateway('inet6')
Gateway-->>Shell: Return IPv6 gateway (optional)
Shell->>GTest: Execute gtest with parameters
Note over Shell,GTest: --addr=<ips> -r <non-routable> -g <routable>
GTest->>Main: Parse command line arguments
Main->>Main: _set_config() processes -r and -g options
Main->>Main: Call sys_get_addr() for remote_addr
Main->>Main: Call sys_get_addr() for remote_routable_addr
Main->>Main: set_def_remote_address()
Main->>Main: Set port 8888 on both addresses
Main->>TestBase: Initialize gtest_conf structure
TestBase->>TestBase: Constructor copies addresses
TestBase->>TestBase: convert_and_copy_address(remote_addr)
TestBase->>TestBase: convert_and_copy_address(remote_routable_addr)
TestBase->>Test: Run test case
alt Bind failure test (tcp_bind.ti_2, udp_bind.ti_2)
Test->>Test: Use remote_addr (non-routable)
Test->>Test: bind() should fail with EADDRNOTAVAIL
else Connect/routing test (tcp_event.ti_2, udp_connect.ti_2)
Test->>Test: Use remote_routable_addr (gateway)
Test->>Test: connect() attempts actual routing
Test->>Test: Expect EPOLLERR|EPOLLHUP or success
end
Test-->>Shell: Return test result
end
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)logic: inverted condition – function returns early when ptr is non-null, so nothing is dumped. Line 24 should be
if (!ptr)to match the intent
11 files reviewed, 2 comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the gtest remote address architecture to support containerized environments by splitting remote_addr into two fields: remote_addr (non-routable, e.g., 74.190.183.38 for bind-failure tests) and remote_routable_addr (actual gateway for connect tests). Gateway detection moves from C++ (sys_gateway()) to the bash script layer (do_get_gateway() in gtest.sh), which invokes the route command at test launch time and passes addresses via new -r and -g flags. The def_gw_exists boolean flag and associated skip logic (SKIP_TRUE) are removed from test classes. Tests like tcp_event::ti_2 and udp_connect::ti_2 now unconditionally use remote_routable_addr for connect operations, relying on the shell script to provide valid gateway IPs. A helper function convert_and_copy_address() is introduced in test_base to reduce IPv4-to-IPv6 address-conversion boilerplate.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
tests/gtest/common/sys.cc |
1/5 | Critical bug introduced: lines 24-26 invert null-check, breaking sys_hexdump(). Removes sys_gateway() function to move gateway detection to shell layer. |
contrib/jenkins_tests/gtest.sh |
3/5 | Adds do_get_gateway() bash function; enforces IPv4 gateway presence (exits if missing) but allows IPv6 to proceed without gateway; grep pattern may miss some route flags. |
tests/gtest/main.cc |
3/5 | Adds -g flag parsing for routable address; set_def_remote_address() unconditionally overwrites ports to 8888, potentially clobbering user-supplied ports in -r/-g arguments. |
tests/gtest/udp/udp_connect.cc |
3/5 | Replaces remote_addr with remote_routable_addr and removes skip logic; uninitialized address if shell script fails to pass -g will cause silent test failures. |
tests/gtest/tcp/tcp_bind.cc |
4/5 | Removes SKIP_TRUE check; test now runs unconditionally relying on shell script to provide valid non-routable address via -r. |
tests/gtest/common/base.cc |
4/5 | Introduces helper function convert_and_copy_address() to reduce duplication; adds remote_routable_addr field initialization; depends on shell script passing -g or field remains uninitialized. |
tests/gtest/tcp/tcp_event.cc |
4/5 | Switches to remote_routable_addr and removes skip check; uses sizeof(remote_routable_addr) which passes extra padding for IPv4 addresses. |
tests/gtest/common/base.h |
5/5 | Adds remote_routable_addr field and removes def_gw_exists flag; structural change with no runtime issues. |
tests/gtest/common/def.h |
5/5 | Adds remote_routable_addr to gtest_configure_t struct and removes def_gw_exists flag; clean structural change. |
tests/gtest/common/sys.h |
5/5 | Removes sys_gateway() function declaration; minimal header change consistent with architectural refactor. |
tests/gtest/udp/udp_bind.cc |
5/5 | Removes SKIP_TRUE check; test now runs unconditionally expecting non-routable address from -r flag. |
Confidence score: 1/5
- This PR introduces a critical bug in
sys_hexdump()and has multiple logic issues that will cause tests to fail or behave unpredictably in production. - Score is 1/5 due to: (1) broken
sys_hexdump()function with inverted null-check preventing any hex output; (2) uninitializedremote_routable_addrwhen-gflag is not provided by shell script, causing tests to read garbage memory; (3) unconditional port overwriting inset_def_remote_address()that may clobber user-specified ports; (4) IPv4 gateway detection exit prevents IPv6 tests from running even though they're designed to work without gateways; (5) grep pattern limitations indo_get_gateway()may miss valid route entries with flags like UGH/UGL. - Pay close attention to
tests/gtest/common/sys.cc(sys_hexdump bug),contrib/jenkins_tests/gtest.sh(gateway detection exit strategy),tests/gtest/main.cc(port handling and uninitialized address defaults), and all test files usingremote_routable_addr(tcp_event.cc, udp_connect.cc) for uninitialized memory reads.
Sequence Diagram
sequenceDiagram
participant CI as "CI Pipeline"
participant Shell as "gtest.sh"
participant Route as "route command"
participant Main as "gtest main()"
participant Config as "gtest_configure_t"
participant Base as "test_base"
participant Tests as "Test Cases"
CI->>Shell: "Execute gtest.sh"
Shell->>Route: "do_get_gateway('inet')"
Route-->>Shell: "IPv4 gateway address"
Shell->>Route: "do_get_gateway('inet6')"
Route-->>Shell: "IPv6 gateway address"
alt No IPv4 gateway found
Shell->>CI: "exit 1 (FAIL)"
end
Shell->>Main: "gtest --addr=... -r 74.190.183.38 -g <gateway_ipv4>"
Main->>Config: "_def_config()"
Config-->>Main: "Default addresses set"
Main->>Config: "_set_config(argc, argv)"
Config->>Config: "Parse -r (remote_addr)"
Config->>Config: "Parse -g (remote_routable_addr)"
Config->>Config: "set_def_remote_address()"
Config-->>Main: "Configuration complete"
Main->>Base: "test_base() constructor"
Base->>Base: "convert_and_copy_address(gtest_conf.remote_addr, remote_addr)"
Base->>Base: "convert_and_copy_address(gtest_conf.remote_routable_addr, remote_routable_addr)"
Base-->>Main: "Base initialized"
Main->>Tests: "RUN_ALL_TESTS()"
Tests->>Tests: "tcp_bind.ti_2 / udp_bind.ti_2"
Tests->>Tests: "bind(fd, remote_addr) [non-routable]"
Tests->>Tests: "Expect EADDRNOTAVAIL"
Tests->>Tests: "tcp_event.ti_2 / udp_connect.ti_2"
Tests->>Tests: "connect(fd, remote_routable_addr) [gateway]"
Tests->>Tests: "Expect routing to work"
Tests-->>Main: "Test results"
Main-->>Shell: "Exit code"
Shell-->>CI: "Overall test status"
Additional Comments (1)
-
tests/gtest/common/sys.cc, line 24-26 (link)logic: should check
!ptrnotptr. This condition returns early when ptr is valid, so hexdump never executes. Lines 25-26 should beif (!ptr) { log_trace(...); return; }
11 files reviewed, 1 comment
Split the remote address configuration into two separate addresses to properly test different network scenarios: 1. remote_addr (-r): Non-routable address for bind failure tests (e.g., 74.190.183.38 or fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff) Used in tcp_bind.ti_2 and udp_bind.ti_2 to verify EADDRNOTAVAIL 2. remote_routable_addr (-g): Routable address for connect tests (dynamically detected default gateway or manually specified) Used in tcp_event.ti_2, udp_connect.ti_2 to test actual routing Remove the def_gw_exists flag and sys_gateway() C++ function, moving gateway detection to the shell script layer using do_get_gateway(). This bash function mirrors the original C++ logic using route command to detect default gateway for both IPv4 and IPv6. Update gtest.sh to dynamically detect system default, making tests more portable across different network environments. Gateway detection happens at test invocation time rather than during test execution. Remove SKIP_TRUE(def_gw_exists) checks from tests as gateway detection is now handled by the shell script which passes the appropriate address via command-line arguments. Tests requiring routable addresses will receive the actual gateway IP, while bind failure tests continue using non-routable dummy addresses. Changes: - Add remote_routable_addr to gtest_configure_t structure - Add -g/--remote-routable command-line option to gtest - Implement do_get_gateway() bash function in gtest.sh - Update tcp_event and udp_connect tests to use remote_routable_addr - Remove sys_gateway() function from common/sys.cc - Remove def_gw_exists flag from test_base class - Remove gateway detection skip logic from individual tests Signed-off-by: Tomer Cabouly <[email protected]>
|
better approach was taken - see #500 |
2 participants
Description
Split the remote address configuration into two separate addresses to properly test different network scenarios:
remote_addr (-r): Non-routable address for bind failure tests (e.g., 74.190.183.38 or fdff:ffff:ffff:ffff:ffff:ffff:ffff:ffff) Used in tcp_bind.ti_2 and udp_bind.ti_2 to verify EADDRNOTAVAIL
remote_routable_addr (-g): Routable address for connect tests (dynamically detected default gateway or manually specified) Used in tcp_event.ti_2, udp_connect.ti_2 to test actual routing
Remove the def_gw_exists flag and sys_gateway() C++ function, moving gateway detection to the shell script layer using do_get_gateway(). This bash function mirrors the original C++ logic using route command to detect default gateway for both IPv4 and IPv6.
Update gtest.sh to dynamically detect system default, making tests more portable across different network environments. Gateway detection happens at test invocation time rather than during test execution.
Remove SKIP_TRUE(def_gw_exists) checks from tests as gateway detection is now handled by the shell script which passes the appropriate address via command-line arguments. Tests requiring routable addresses will receive the actual gateway IP, while bind failure tests continue using non-routable dummy addresses.
Changes:
What
gtest: distinguish routable and non-routable remote addresses
Why ?
support gtest container migration, as current spawned containers don't have gateways.
How ?
It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.
Change type
What kind of change does this PR introduce?
Check list