fix(pal): MSVC/Windows portability for socket calls and macros#70
Merged
Conversation
Extend the Platform Abstraction Layer with portable wrappers for socket functions whose signatures differ between POSIX (void*) and Winsock (char*), and for errno-based error reporting (errno vs WSAGetLastError). Changes: - CMakeLists.txt: add NOMINMAX to Windows compile definitions alongside NOGDI and WIN32_LEAN_AND_MEAN (fixes std::min/std::max macro conflict) - win32/net_impl.h: add ssize_t typedef, someip_setsockopt/getsockopt/ sendto/recvfrom/send/recv wrappers with reinterpret_cast, socket timeout helper (DWORD ms vs struct timeval), someip_socket_errno() returning WSAGetLastError(), and SOMEIP_E* error constant mappings - posix/net_impl.h, lwip/net_impl.h, zephyr/net_impl.h: add matching pass-through wrappers so transport code compiles on all platforms - udp_transport.cpp: replace raw setsockopt/sendto/recvfrom/errno with someip_* portable wrappers - tcp_transport.cpp: same treatment plus portable socket timeouts via someip_set_socket_timeout(), Windows-compatible TCP keepalive, and someip_socket_errno() for non-blocking connect error checking All 15 existing tests pass. Closes #69 Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughThe PR introduces cross-platform socket operation abstractions and error handling wrappers for POSIX, Windows (Winsock), lwIP, and Zephyr platforms. Transport layer implementations are updated to use these wrappers, and Windows compiler flags are added to resolve MSVC compatibility issues. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Requirements Validation ReportRequirements Change Summary
Added Requirements
Validation Details |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/platform/posix/net_impl.h`:
- Around line 58-66: The header uses errno and POSIX error macros but doesn't
include errno.h; make the header self-contained by adding an explicit `#include`
<errno.h> at the top of include/platform/posix/net_impl.h so
someip_socket_errno() and the macros SOMEIP_EAGAIN, SOMEIP_EWOULDBLOCK,
SOMEIP_EINPROGRESS, SOMEIP_EBADF, SOMEIP_EINTR resolve reliably; ensure the
include appears before the declaration of static inline int
someip_socket_errno() and the macro definitions.
In `@include/platform/win32/net_impl.h`:
- Around line 33-74: Change all socket handle parameters and storage from int to
Winsock SOCKET to avoid 32-bit truncation: update functions someip_setsockopt,
someip_getsockopt, someip_sendto, someip_recvfrom, someip_send, someip_recv to
accept SOCKET instead of int and adjust any casts accordingly (e.g., keep
reinterpret_cast on buffer pointers, but pass the SOCKET directly to
setsockopt/getsockopt/send/recv/sendto/recvfrom). Also update transport-layer
types that store handles (references in udp_transport.cpp and tcp_transport.cpp)
from int to SOCKET, and ensure winsock2.h is included where SOCKET is used and
any socklen_t/int conversions are handled safely. Ensure builds compile with the
new signatures across all call sites.
In `@src/transport/tcp_transport.cpp`:
- Around line 483-488: In send_data() and receive_data(), extend the error
handling after someip_socket_errno() to treat SOMEIP_EINTR like
SOMEIP_EAGAIN/SOMEIP_EWOULDBLOCK by continuing and retrying the operation
instead of returning Result::NETWORK_ERROR; specifically, update the branches
that currently only check SOMEIP_EAGAIN/SOMEIP_EWOULDBLOCK to also check for
SOMEIP_EINTR (using the existing someip_socket_errno() call and the SOMEIP_EINTR
constant), so interrupted syscalls are retried rather than forcing a disconnect.
- Around line 337-340: The call to someip_getsockopt(...) that fetches SO_ERROR
isn't checking the function's return value, so if someip_getsockopt fails you
still treat the connection as successful; update the code around the
someip_getsockopt call (the block using connection_.socket_fd, SOL_SOCKET,
SO_ERROR) to capture its return value, and if the return is non-zero treat the
operation as a failure (populate/override error with a meaningful non-zero value
such as errno or a platform-specific error code) and handle that path the same
as a socket error instead of assuming success.
- Around line 271-285: The keepalive setup is incorrect: enable SO_KEEPALIVE on
non-Windows paths and pass the idle timeout value (keep_alive_interval) to
TCP_KEEPALIVE/TCP_KEEPIDLE instead of the boolean keep_alive; update the code
around someip_setsockopt/socket_fd in tcp_transport.cpp to call
someip_setsockopt(socket_fd, SOL_SOCKET, SO_KEEPALIVE, &keep_alive,
sizeof(keep_alive)) for macOS/Linux, then call someip_setsockopt with
IPPROTO_TCP and TCP_KEEPALIVE (macOS) or TCP_KEEPIDLE (Linux) using
&keep_alive_interval, and keep using TCP_KEEPINTVL and TCP_KEEPCNT with
keep_alive_interval and keep_alive respectively (and still noop
keep_alive_interval on Windows).
In `@src/transport/udp_transport.cpp`:
- Around line 423-431: The receive loop treats SOMEIP_EINTR as NOT_CONNECTED
which prematurely exits the receiver; change the error handling in the
recvfrom() error branch in udp_transport's receive_loop so that ONLY
SOMEIP_EBADF returns Result::NOT_CONNECTED, while SOMEIP_EINTR causes the loop
to retry (i.e., continue to the next recv attempt); keep the existing handling
for non-blocking EAGAIN/EWOULDBLOCK returning Result::TIMEOUT and preserve use
of someip_socket_errno(), Result::NOT_CONNECTED, Result::TIMEOUT and
config_.blocking so behavior for other cases is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3dc418f-3814-4476-a0de-ddddb3408222
📒 Files selected for processing (7)
include/platform/lwip/net_impl.hinclude/platform/posix/net_impl.hinclude/platform/win32/net_impl.hinclude/platform/zephyr/net_impl.hsrc/CMakeLists.txtsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpp
Comment on lines
+58
to
+66
| /* ---------- Error reporting (pass-through on POSIX) ------------------------ */ | ||
|
|
||
| static inline int someip_socket_errno() { return errno; } | ||
|
|
||
| #define SOMEIP_EAGAIN EAGAIN | ||
| #define SOMEIP_EWOULDBLOCK EWOULDBLOCK | ||
| #define SOMEIP_EINPROGRESS EINPROGRESS | ||
| #define SOMEIP_EBADF EBADF | ||
| #define SOMEIP_EINTR EINTR |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n include/platform/posix/net_impl.h | head -80Repository: vtz/opensomeip
Length of output: 2703
🏁 Script executed:
grep -r "SOMEIP_E\|someip_socket_errno" src/transport/ --include="*.cpp"Repository: vtz/opensomeip
Length of output: 782
🏁 Script executed:
# Check if errno.h is included transitively through any of the currently included headers
# by checking the standard POSIX header dependencies
cat > /tmp/check_errno.c << 'EOF'
`#include` <sys/socket.h>
`#include` <sys/select.h>
`#include` <netinet/in.h>
`#include` <netinet/tcp.h>
`#include` <arpa/inet.h>
`#include` <unistd.h>
`#include` <fcntl.h>
int main() {
// Try to use errno and E* constants without explicit errno.h
int x = errno;
int y = EAGAIN;
return 0;
}
EOF
# Just check if the header will compile as-is
head -20 include/platform/posix/net_impl.h > /tmp/test_header.h
</script>Repository: vtz/opensomeip
Length of output: 162
🏁 Script executed:
grep -B5 "SOMEIP_E\|someip_socket_errno" src/transport/tcp_transport.cpp | head -20Repository: vtz/opensomeip
Length of output: 689
🏁 Script executed:
head -30 src/transport/tcp_transport.cpp | grep -E "#include"Repository: vtz/opensomeip
Length of output: 254
🏁 Script executed:
cat include/platform/net.hRepository: vtz/opensomeip
Length of output: 1039
🏁 Script executed:
# Check if other platform headers include errno.h or have similar patterns
find include/platform -name "*.h" -type f | xargs grep -l "errno"Repository: vtz/opensomeip
Length of output: 193
🏁 Script executed:
# Check how other platforms handle errno/error codes
for file in include/platform/lwip/net_impl.h include/platform/win32/net_impl.h include/platform/zephyr/net_impl.h; do
echo "=== $file ==="
head -30 "$file" | grep -E "#include|errno"
doneRepository: vtz/opensomeip
Length of output: 489
🏁 Script executed:
# Check if any of the currently included headers in POSIX net_impl.h transitively include errno.h
# by searching for errno in the included headers' documentation or checking standard POSIX behavior
# Let's search for how errno is declared in standard headers
grep -r "define errno\|extern.*errno" /usr/include/ 2>/dev/null | grep -E "errno.h|sys/socket.h|unistd.h" | head -5Repository: vtz/opensomeip
Length of output: 40
🏁 Script executed:
# Let's check what the C99/POSIX standard says about errno and these error codes
# by looking at a simple test: can we use errno and EAGAIN without errno.h?
# We'll create a minimal test file that mimics the current header
cat > /tmp/test_errno_includes.c << 'EOF'
`#include` <sys/socket.h>
`#include` <sys/select.h>
`#include` <netinet/in.h>
`#include` <netinet/tcp.h>
`#include` <arpa/inet.h>
`#include` <unistd.h>
`#include` <fcntl.h>
/* This mimics the problematic lines in net_impl.h */
int test_errno_usage() {
return errno; /* Line 60 equivalent */
}
int test_error_codes() {
int codes[] = {EAGAIN, EWOULDBLOCK, EINPROGRESS, EBADF, EINTR};
return codes[0];
}
EOF
# Try to compile it to see if we get warnings/errors about undefined symbols
gcc -c /tmp/test_errno_includes.c -o /tmp/test_errno_includes.o 2>&1 | head -20Repository: vtz/opensomeip
Length of output: 100
🏁 Script executed:
# Check if any other source files in the codebase include errno.h to understand the pattern
grep -r "#include.*errno" --include="*.h" --include="*.cpp" --include="*.c" .Repository: vtz/opensomeip
Length of output: 181
Include the errno declarations explicitly in this header.
Line 60 and lines 62-66 use errno and the error constants (EAGAIN, EWOULDBLOCK, EINPROGRESS, EBADF, EINTR), but this header does not explicitly include <errno.h>. While this may compile due to transitive inclusion through other headers (which is implementation-dependent), the header should be self-contained. The POSIX backend is currently include-order dependent.
Suggested fix
`#include` <arpa/inet.h>
`#include` <unistd.h>
`#include` <fcntl.h>
+#include <errno.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/platform/posix/net_impl.h` around lines 58 - 66, The header uses
errno and POSIX error macros but doesn't include errno.h; make the header
self-contained by adding an explicit `#include` <errno.h> at the top of
include/platform/posix/net_impl.h so someip_socket_errno() and the macros
SOMEIP_EAGAIN, SOMEIP_EWOULDBLOCK, SOMEIP_EINPROGRESS, SOMEIP_EBADF,
SOMEIP_EINTR resolve reliably; ensure the include appears before the declaration
of static inline int someip_socket_errno() and the macro definitions.
Comment on lines
+33
to
+74
| static inline int someip_setsockopt(int fd, int level, int optname, | ||
| const void* optval, int optlen) { | ||
| return setsockopt(fd, level, optname, | ||
| reinterpret_cast<const char*>(optval), optlen); | ||
| } | ||
|
|
||
| /** @implements REQ_PAL_NET_SOCKOPT */ | ||
| static inline int someip_getsockopt(int fd, int level, int optname, | ||
| void* optval, socklen_t* optlen) { | ||
| return getsockopt(fd, level, optname, | ||
| reinterpret_cast<char*>(optval), | ||
| reinterpret_cast<int*>(optlen)); | ||
| } | ||
|
|
||
| /** @implements REQ_PAL_NET_SEND */ | ||
| static inline ssize_t someip_sendto(int fd, const void* buf, size_t len, | ||
| int flags, const struct sockaddr* dest, | ||
| socklen_t addrlen) { | ||
| return sendto(fd, reinterpret_cast<const char*>(buf), | ||
| static_cast<int>(len), flags, dest, addrlen); | ||
| } | ||
|
|
||
| /** @implements REQ_PAL_NET_RECV */ | ||
| static inline ssize_t someip_recvfrom(int fd, void* buf, size_t len, | ||
| int flags, struct sockaddr* src, | ||
| socklen_t* addrlen) { | ||
| return recvfrom(fd, reinterpret_cast<char*>(buf), | ||
| static_cast<int>(len), flags, src, addrlen); | ||
| } | ||
|
|
||
| /** @implements REQ_PAL_NET_SEND */ | ||
| static inline ssize_t someip_send(int fd, const void* buf, size_t len, | ||
| int flags) { | ||
| return send(fd, reinterpret_cast<const char*>(buf), | ||
| static_cast<int>(len), flags); | ||
| } | ||
|
|
||
| /** @implements REQ_PAL_NET_RECV */ | ||
| static inline ssize_t someip_recv(int fd, void* buf, size_t len, int flags) { | ||
| return recv(fd, reinterpret_cast<char*>(buf), | ||
| static_cast<int>(len), flags); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the net_impl.h file structure
head -75 include/platform/win32/net_impl.h | tail -45Repository: vtz/opensomeip
Length of output: 1882
🏁 Script executed:
# Search for socket handle declarations in transport layer
rg -A 3 -B 1 "int.*fd|int.*socket" src/transport/udp_transport.cpp | head -50Repository: vtz/opensomeip
Length of output: 223
🏁 Script executed:
# Check tcp_transport.cpp socket usage
rg -A 3 -B 1 "int.*fd|int.*socket" src/transport/tcp_transport.cpp | head -50Repository: vtz/opensomeip
Length of output: 1963
🏁 Script executed:
# Verify specific lines mentioned in the review
sed -n '240,250p' src/transport/udp_transport.cpp
sed -n '210,235p' src/transport/tcp_transport.cppRepository: vtz/opensomeip
Length of output: 925
🏁 Script executed:
# Search for SOCKET type or Winsock usage in the codebase
rg "SOCKET\s*\b" --type cpp include/ src/Repository: vtz/opensomeip
Length of output: 1395
🌐 Web query:
Winsock SOCKET type definition 64-bit Windows UINT_PTR
💡 Result:
On Windows, Winsock’s SOCKET is defined as a pointer-sized unsigned integer:
- In
winsock2.h:typedef UINT_PTR SOCKET;[1] UINT_PTRis an unsigned integer large enough to hold a pointer; on 64-bit Windows it is 64 bits (e.g.,typedef unsigned __int64 UINT_PTR;) [2]
So on 64-bit Windows, SOCKET is a 64-bit unsigned integral type (pointer-width), not an int. [1][2]
🌐 Web query:
Windows setsockopt getsockopt SOCKET type vs int parameter
💡 Result:
On Windows (Winsock), the “socket” is not an int file descriptor like POSIX. It’s a distinct handle type:
SOCKETis typedef’d as a pointer-sized unsigned integer (UINT_PTR), so it’s 32-bit on x86 and 64-bit on x64. (doxygen.reactos.org)- Therefore
setsockopt/getsockopttake aSOCKET sas their first parameter (the socket handle), notint. (learn.microsoft.com)
The option length parameters are still int in Winsock:
setsockopt(..., const char *optval, int optlen)usesoptlenby value. (learn.microsoft.com)getsockopt(..., char *optval, int *optlen)usesoptlenas an in/out pointer. (learn.microsoft.com)
Why char*/const char* for optval? It’s historical/ABI compatibility: options are treated as a typeless byte buffer, so you pass the address of your typed value and cast it to (const char*) / (char*). (learn.microsoft.com)
Practical rules:
- Always pass the correct size:
optlen = sizeof(value)(many “boolean” options expectsizeof(int)in Winsock). (learn.microsoft.com) - Don’t assume POSIX types (
socklen_t,timeval, etc.) match Winsock—follow the specific option’s documented type/size. (learn.microsoft.com)
Citations:
- 1: https://doxygen.reactos.org/da/de8/winsock_8h_source.html?utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-setsockopt?utm_source=openai
- 3: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-setsockopt?utm_source=openai
- 4: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-getsockopt?utm_source=openai
- 5: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-setsockopt?utm_source=openai
- 6: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-setsockopt?utm_source=openai
- 7: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-getsockopt?utm_source=openai
🏁 Script executed:
# Check if there's any `#include` <winsock2.h> or similar in the Windows PAL
rg "winsock|Windows\.h|ws2_32" include/platform/win32/ src/Repository: vtz/opensomeip
Length of output: 244
🏁 Script executed:
# Check what the actual socket variable types are in transport layer
rg "socket_fd_|listen_socket_fd_|client_fd" --type cpp src/transport/ | grep -E "(int|SOCKET)" | head -20Repository: vtz/opensomeip
Length of output: 1402
Use Winsock SOCKET type for socket handles, not int.
The wrapper functions take int fd, but Winsock's SOCKET is typedef'd as UINT_PTR—a 64-bit unsigned integer on 64-bit Windows. Passing a 64-bit handle into an int parameter truncates it to 32-bit, corrupting the socket before each API call. The transport layer (udp_transport.cpp, tcp_transport.cpp) also stores handles in int, so the type mismatch propagates throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/platform/win32/net_impl.h` around lines 33 - 74, Change all socket
handle parameters and storage from int to Winsock SOCKET to avoid 32-bit
truncation: update functions someip_setsockopt, someip_getsockopt,
someip_sendto, someip_recvfrom, someip_send, someip_recv to accept SOCKET
instead of int and adjust any casts accordingly (e.g., keep reinterpret_cast on
buffer pointers, but pass the SOCKET directly to
setsockopt/getsockopt/send/recv/sendto/recvfrom). Also update transport-layer
types that store handles (references in udp_transport.cpp and tcp_transport.cpp)
from int to SOCKET, and ensure winsock2.h is included where SOCKET is used and
any socklen_t/int conversions are handled safely. Ensure builds compile with the
new signatures across all call sites.
Comment on lines
271
to
285
| int keep_alive = 1; | ||
| #ifdef __APPLE__ | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPALIVE, &keep_alive, sizeof(keep_alive)); | ||
| int keep_alive_interval = static_cast<int>(config_.keep_alive_interval.count() / 1000); | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPINTVL, &keep_alive_interval, sizeof(keep_alive_interval)); | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_alive, sizeof(keep_alive)); | ||
| #if defined(__APPLE__) | ||
| someip_setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPALIVE, &keep_alive, sizeof(keep_alive)); | ||
| #elif defined(_WIN32) | ||
| someip_setsockopt(socket_fd, SOL_SOCKET, SO_KEEPALIVE, &keep_alive, sizeof(keep_alive)); | ||
| #else | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPIDLE, &keep_alive, sizeof(keep_alive)); | ||
| int keep_alive_interval = static_cast<int>(config_.keep_alive_interval.count() / 1000); | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPINTVL, &keep_alive_interval, sizeof(keep_alive_interval)); | ||
| setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_alive, sizeof(keep_alive)); | ||
| someip_setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPIDLE, &keep_alive, sizeof(keep_alive)); | ||
| #endif | ||
| #if !defined(_WIN32) | ||
| someip_setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPINTVL, &keep_alive_interval, sizeof(keep_alive_interval)); | ||
| someip_setsockopt(socket_fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_alive, sizeof(keep_alive)); | ||
| #else | ||
| (void)keep_alive_interval; | ||
| #endif |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "tcp_transport.cpp" -type fRepository: vtz/opensomeip
Length of output: 91
🏁 Script executed:
cat -n src/transport/tcp_transport.cpp | sed -n '271,285p'Repository: vtz/opensomeip
Length of output: 921
🏁 Script executed:
# Search for any documentation or comments about keepalive setup
rg "TCP_KEEP|SO_KEEPALIVE" -A 2 -B 2Repository: vtz/opensomeip
Length of output: 1197
🏁 Script executed:
# Look for any existing documentation or comments about SO_KEEPALIVE
rg "SO_KEEPALIVE" -B 5 -A 5Repository: vtz/opensomeip
Length of output: 1023
🏁 Script executed:
# Check if there are any man pages or documentation files
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | head -20Repository: vtz/opensomeip
Length of output: 539
🏁 Script executed:
# Search for socket option constant definitions
rg "TCP_KEEPALIVE|TCP_KEEPIDLE|TCP_KEEPINTVL|TCP_KEEPCNT" | head -20Repository: vtz/opensomeip
Length of output: 596
🏁 Script executed:
# Look at broader context around this keepalive setup to understand the flow
cat -n src/transport/tcp_transport.cpp | sed -n '260,295p'Repository: vtz/opensomeip
Length of output: 1718
🌐 Web query:
TCP_KEEPIDLE TCP_KEEPALIVE socket option units seconds milliseconds
💡 Result:
- Linux:
TCP_KEEPIDLE(a.k.a.tcp_keepalive_time) is in seconds. (man7.org) - AIX:
TCP_KEEPIDLEviasetsockopt()is in seconds (even though some system tunables use half-second units). (ibm.com) - Solaris/illumos:
TCP_KEEPIDLEis in seconds, whileTCP_KEEPALIVE_THRESHOLDis in milliseconds. (docs.oracle.com) TCP_KEEPALIVEoption name (where it exists): commonly seconds (e.g., Solaris man page; z/OS docs). (docs.oracle.com)
If you tell me your target OS (Linux/macOS/FreeBSD/Solaris/Windows), I can map the exact option names you should use there and their units.
Citations:
- 1: https://man7.org/linux/man-pages/man7/tcp.7.html?utm_source=openai
- 2: https://www.ibm.com/support/pages/ibm-aix-tcp-keepalive-probes?utm_source=openai
- 3: https://docs.oracle.com/cd/E88353_01/html/E37851/tcp-4p.html?utm_source=openai
- 4: https://docs.oracle.com/cd/E86824_01/html/E54777/tcp-7p.html?utm_source=openai
🌐 Web query:
SO_KEEPALIVE must be set before TCP_KEEPIDLE TCP_KEEPINTVL
💡 Result:
On Linux, TCP_KEEPIDLE and TCP_KEEPINTVL only take effect for a socket when SO_KEEPALIVE is enabled (they control when/ how often keepalive probes are sent if SO_KEEPALIVE is set). They are effectively no-ops while SO_KEEPALIVE is off. [1]
However, the Linux API does not require a strict setsockopt call order (i.e., it’s not “must set SO_KEEPALIVE before TCP_KEEPIDLE/TCP_KEEPINTVL”); the practical requirement is simply: enable SO_KEEPALIVE at some point, otherwise no keepalive probes will be sent. [1][2]
Common pattern (for clarity/portability):
setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, ...)setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, ...)setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, ...)(and optionallyTCP_KEEPCNT) [2]
Sources:
[1] Linux tcp(7) man page (TCP_KEEPIDLE “... if the socket option SO_KEEPALIVE has been set ...”)
[2] TCP Keepalive HOWTO (enabling with SO_KEEPALIVE, and per-socket overrides via TCP_KEEPIDLE/TCP_KEEPINTVL)
SO_KEEPALIVE must be enabled on macOS/Linux for keepalive to work, and the idle timeout options receive the wrong values.
Line 274 (macOS) and line 278 (Linux) pass keep_alive (value 1) to TCP_KEEPALIVE and TCP_KEEPIDLE respectively; these options expect the idle timeout in seconds, so they should receive keep_alive_interval instead. Additionally, the macOS/Linux branches never enable SO_KEEPALIVE at the socket level—without it, the subsequent TCP_KEEP* options have no effect. The Windows branch correctly enables SO_KEEPALIVE (line 276), but the non-Windows paths skip this step entirely, rendering keepalive non-functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transport/tcp_transport.cpp` around lines 271 - 285, The keepalive setup
is incorrect: enable SO_KEEPALIVE on non-Windows paths and pass the idle timeout
value (keep_alive_interval) to TCP_KEEPALIVE/TCP_KEEPIDLE instead of the boolean
keep_alive; update the code around someip_setsockopt/socket_fd in
tcp_transport.cpp to call someip_setsockopt(socket_fd, SOL_SOCKET, SO_KEEPALIVE,
&keep_alive, sizeof(keep_alive)) for macOS/Linux, then call someip_setsockopt
with IPPROTO_TCP and TCP_KEEPALIVE (macOS) or TCP_KEEPIDLE (Linux) using
&keep_alive_interval, and keep using TCP_KEEPINTVL and TCP_KEEPCNT with
keep_alive_interval and keep_alive respectively (and still noop
keep_alive_interval on Windows).
Comment on lines
337
to
340
| int error = 0; | ||
| socklen_t len = sizeof(error); | ||
| getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len); | ||
| someip_getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/transport/tcp_transport.cppRepository: vtz/opensomeip
Length of output: 93
🏁 Script executed:
sed -n '330,350p' src/transport/tcp_transport.cppRepository: vtz/opensomeip
Length of output: 949
🏁 Script executed:
sed -n '320,360p' src/transport/tcp_transport.cppRepository: vtz/opensomeip
Length of output: 1517
🏁 Script executed:
rg "someip_getsockopt" --type cppRepository: vtz/opensomeip
Length of output: 497
🏁 Script executed:
rg "int someip_getsockopt" -A 3 --type cppRepository: vtz/opensomeip
Length of output: 429
🏁 Script executed:
fd . -name "*.h" -o -name "*.hpp" | xargs rg "someip_getsockopt" -B 2 -A 5Repository: vtz/opensomeip
Length of output: 2936
🏁 Script executed:
sed -n '310,370p' src/transport/tcp_transport.cppRepository: vtz/opensomeip
Length of output: 2171
🏁 Script executed:
rg "disconnect_internal" src/transport/tcp_transport.cpp -B 2 -A 2Repository: vtz/opensomeip
Length of output: 718
Check someip_getsockopt() return value before trusting SO_ERROR.
The someip_getsockopt() call at line 339 ignores the return value. On all platforms (POSIX, Windows, Zephyr, lwip), someip_getsockopt returns 0 on success and non-zero on failure. If getsockopt(SO_ERROR) itself fails, the error variable stays at its initialized value of 0, causing the code to incorrectly report the connection as SUCCESS.
Suggested fix
int error = 0;
socklen_t len = sizeof(error);
- someip_getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len);
+ if (someip_getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len) < 0) {
+ disconnect_internal();
+ return Result::NETWORK_ERROR;
+ }
if (error == 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int error = 0; | |
| socklen_t len = sizeof(error); | |
| getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len); | |
| someip_getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len); | |
| int error = 0; | |
| socklen_t len = sizeof(error); | |
| if (someip_getsockopt(connection_.socket_fd, SOL_SOCKET, SO_ERROR, &error, &len) < 0) { | |
| disconnect_internal(); | |
| return Result::NETWORK_ERROR; | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transport/tcp_transport.cpp` around lines 337 - 340, The call to
someip_getsockopt(...) that fetches SO_ERROR isn't checking the function's
return value, so if someip_getsockopt fails you still treat the connection as
successful; update the code around the someip_getsockopt call (the block using
connection_.socket_fd, SOL_SOCKET, SO_ERROR) to capture its return value, and if
the return is non-zero treat the operation as a failure (populate/override error
with a meaningful non-zero value such as errno or a platform-specific error
code) and handle that path the same as a socket error instead of assuming
success.
Comment on lines
483
to
488
| if (sent < 0) { | ||
| if (errno == EAGAIN || errno == EWOULDBLOCK) { | ||
| int err = someip_socket_errno(); | ||
| if (err == SOMEIP_EAGAIN || err == SOMEIP_EWOULDBLOCK) { | ||
| continue; // Retry | ||
| } | ||
| return Result::NETWORK_ERROR; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the tcp_transport.cpp file
find . -name "tcp_transport.cpp" -type fRepository: vtz/opensomeip
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the relevant sections around lines 483-488 and 511-515
# Let's get a broader context to understand the code structure
fd tcp_transport.cpp -x wc -l {}Repository: vtz/opensomeip
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the file with line numbers to examine the error handling
# Get lines 475-495 for the first location
fd tcp_transport.cpp -x cat -n {} | sed -n '475,495p'Repository: vtz/opensomeip
Length of output: 1111
🏁 Script executed:
#!/bin/bash
# Get lines 505-520 for the second location
fd tcp_transport.cpp -x cat -n {} | sed -n '505,520p'Repository: vtz/opensomeip
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Search for SOMEIP_EINTR definition and usage in the codebase
rg "SOMEIP_EINTR" --type-list | head -20Repository: vtz/opensomeip
Length of output: 447
🏁 Script executed:
#!/bin/bash
# Search for SOMEIP_EINTR
rg "SOMEIP_EINTR" -A 2 -B 2Repository: vtz/opensomeip
Length of output: 1699
🏁 Script executed:
#!/bin/bash
# Read the actual tcp_transport.cpp implementation (not the test file)
cat -n ./src/transport/tcp_transport.cpp | sed -n '475,520p'Repository: vtz/opensomeip
Length of output: 2054
🏁 Script executed:
#!/bin/bash
# Search for send() method and error handling in tcp_transport.cpp
rg "if.*sent.*<.*0" ./src/transport/tcp_transport.cpp -A 5 -B 2Repository: vtz/opensomeip
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Also search for the send() calls to understand the context
rg "someip_send|ssize_t.*sent" ./src/transport/tcp_transport.cpp -B 2 -A 5Repository: vtz/opensomeip
Length of output: 368
Retry SOMEIP_EINTR in both TCP I/O paths.
Both send_data() (line 485) and receive_data() (line 512) only special-case SOMEIP_EAGAIN/SOMEIP_EWOULDBLOCK but not SOMEIP_EINTR. When a signal interrupts send() or recv(), the operation fails with EINTR, which the code currently treats as a fatal NETWORK_ERROR and forces a disconnect. Per POSIX standards, signal-interrupted system calls should be retried. The UDP transport correctly handles this pattern (checking both SOMEIP_EBADF and SOMEIP_EINTR), but TCP does not.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transport/tcp_transport.cpp` around lines 483 - 488, In send_data() and
receive_data(), extend the error handling after someip_socket_errno() to treat
SOMEIP_EINTR like SOMEIP_EAGAIN/SOMEIP_EWOULDBLOCK by continuing and retrying
the operation instead of returning Result::NETWORK_ERROR; specifically, update
the branches that currently only check SOMEIP_EAGAIN/SOMEIP_EWOULDBLOCK to also
check for SOMEIP_EINTR (using the existing someip_socket_errno() call and the
SOMEIP_EINTR constant), so interrupted syscalls are retried rather than forcing
a disconnect.
Comment on lines
423
to
431
| if (received < 0) { | ||
| // Socket was closed during shutdown | ||
| if (errno == EBADF || errno == EINTR) { | ||
| int err = someip_socket_errno(); | ||
|
|
||
| if (err == SOMEIP_EBADF || err == SOMEIP_EINTR) { | ||
| return Result::NOT_CONNECTED; | ||
| } | ||
|
|
||
| // In non-blocking mode, EAGAIN/EWOULDBLOCK means no data available | ||
| if (!config_.blocking && (errno == EAGAIN || errno == EWOULDBLOCK)) { | ||
| if (!config_.blocking && (err == SOMEIP_EAGAIN || err == SOMEIP_EWOULDBLOCK)) { | ||
| return Result::TIMEOUT; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its structure
find . -name "udp_transport.cpp" -type fRepository: vtz/opensomeip
Length of output: 91
🏁 Script executed:
# Read the specific lines mentioned in the review
head -n 450 src/transport/udp_transport.cpp | tail -n +400 | cat -nRepository: vtz/opensomeip
Length of output: 1898
🏁 Script executed:
# Get more context around the error handling
sed -n '400,450p' src/transport/udp_transport.cpp | cat -nRepository: vtz/opensomeip
Length of output: 1898
🏁 Script executed:
# Search for SOMEIP_EINTR definition
rg "SOMEIP_EINTR" --type cpp -B 2 -A 2Repository: vtz/opensomeip
Length of output: 1699
🏁 Script executed:
# Look for the complete receive function context
ast-grep --pattern 'fn receive() {
$$$
}'Repository: vtz/opensomeip
Length of output: 40
🏁 Script executed:
# Search for how EINTR is handled elsewhere in the codebase
rg "EINTR" --type cpp -B 3 -A 3Repository: vtz/opensomeip
Length of output: 2083
🏁 Script executed:
# Find callers of receive_data
rg "receive_data" --type cpp -B 2 -A 2Repository: vtz/opensomeip
Length of output: 2477
🏁 Script executed:
# Look for receive loops or retry patterns
rg "receive_data|receive|EINTR" src/transport/ --type cpp | head -40Repository: vtz/opensomeip
Length of output: 3613
🏁 Script executed:
# Check if there's a transport loop that retries on errors
fd . src/transport --type f -name "*.cpp" -o -name "*.h" | xargs grep -l "receive_data" | head -5Repository: vtz/opensomeip
Length of output: 288
🏁 Script executed:
# Get the full receive_loop function
sed -n '350,400p' src/transport/udp_transport.cppRepository: vtz/opensomeip
Length of output: 2127
🏁 Script executed:
# Check the entire receive_loop to understand how errors are handled
rg "void UdpTransport::receive_loop" -A 40 src/transport/udp_transport.cppRepository: vtz/opensomeip
Length of output: 1755
🏁 Script executed:
# Look at error result handling in the loop
rg "receive_loop\|Result result.*receive_data\|result == Result::" src/transport/udp_transport.cpp -A 3 -B 1Repository: vtz/opensomeip
Length of output: 40
Retry SOMEIP_EINTR instead of exiting the receive loop.
Line 27 maps SOMEIP_EINTR to Result::NOT_CONNECTED, which causes receive_loop() to break and exit the receiver thread. EINTR (interrupted syscall) is transient—it occurs when a signal interrupts recvfrom(), not because the socket is closed. Per POSIX semantics, the syscall should be retried. Only SOMEIP_EBADF (invalid socket) justifies returning NOT_CONNECTED.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transport/udp_transport.cpp` around lines 423 - 431, The receive loop
treats SOMEIP_EINTR as NOT_CONNECTED which prematurely exits the receiver;
change the error handling in the recvfrom() error branch in udp_transport's
receive_loop so that ONLY SOMEIP_EBADF returns Result::NOT_CONNECTED, while
SOMEIP_EINTR causes the loop to retry (i.e., continue to the next recv attempt);
keep the existing handling for non-blocking EAGAIN/EWOULDBLOCK returning
Result::TIMEOUT and preserve use of someip_socket_errno(),
Result::NOT_CONNECTED, Result::TIMEOUT and config_.blocking so behavior for
other cases is unchanged.
This was referenced Mar 12, 2026
This was referenced Mar 24, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
void*vschar*buffer types,errnovsWSAGetLastError(),struct timevalvsDWORDtimeouts)NOMINMAXto Windows CMake compile definitions, preventingstd::min/std::maxmacro conflictssetsockopt/sendto/recvfrom/send/recv/errnocalls in transport code with portablesomeip_*wrappersCloses #69
Files changed
src/CMakeLists.txtNOMINMAXto Windows compile definitionsinclude/platform/win32/net_impl.hssize_ttypedef, socket call wrappers withreinterpret_cast,someip_set_socket_timeout(),someip_socket_errno(),SOMEIP_E*error constantsinclude/platform/posix/net_impl.hinclude/platform/lwip/net_impl.hinclude/platform/zephyr/net_impl.hsrc/transport/udp_transport.cppsomeip_*wrappers for all socket and error callssrc/transport/tcp_transport.cppsomeip_*wrappers + portable timeout/keepalive setupIssues addressed from #69
serializer.cppstd::minmacro conflict →NOMINMAXcompile definitionudp_transport.cppsetsockopttype mismatch →someip_setsockoptwrapperudp_transport.cppssize_tundeclared → typedef inwin32/net_impl.hudp_transport.cppsendto/recvfrombuffer type →someip_sendto/someip_recvfromwrapperstcp_transport.cppsame issues (proactively fixed, not in original issue)Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Chores