feat: S11.07 Windows adapters onto PoolAllocator#408
Conversation
Canonical 3-TU split + SolidSyslogPoolAllocator, mirroring S11.06's PosixMutex migration. Public _Create now takes void; _Destroy keeps its base-handle parameter. The SolidSyslogWindowsMutexStorage typedef and SOLIDSYSLOG_WINDOWS_MUTEX_SIZE enum are gone from the public header. Pool size default 1U, override via SOLIDSYSLOG_WINDOWS_MUTEX_POOL_SIZE. Pool exhaustion returns the shared SolidSyslogNullMutex with ERROR report; Destroy of unknown/stale handle emits WARNING. The BddTargetWindows caller drops mutexStorage; the test file drops the storage local and the HandleEqualsStorageAddress test (no longer meaningful when the slot is library-internal) and gains a Pool TEST_GROUP of 9 tests mirroring SolidSyslogPosixMutexPool.
Canonical 3-TU split + SolidSyslogPoolAllocator. The six file-scope Winsock_* function-pointer test seams (Winsock_socket /sendto /closesocket /connect /setsockopt /getsockopt) stay in WinsockDatagram.c per the C4232 __declspec(dllimport) workaround that production code relies on. The Internal.h header continues to expose those externs to WinsockFake. WinsockDatagram_Initialise explicitly sets Fd = INVALID_SOCKET and Connected = false rather than relying on pool zero-init — SOCKET fd = 0 is a valid handle on Windows, not invalid. Same root cause as the PosixDatagram fix in S11.06; documented in this commit so future TcpStream / FileBlockDevice slot migrations don't re-trip on it. Public _Destroy(void) → _Destroy(base) shape. SOLIDSYSLOG_WINSOCK_DATAGRAM_POOL_SIZE default 1U. Pool exhaustion returns shared SolidSyslogNullDatagram with ERROR; unknown/stale destroy emits WARNING. BddTargetWindows caller updated; test file rewires teardown and gains a Pool TEST_GROUP of 9 tests mirroring SolidSyslogPosixDatagramPool.
Canonical 3-TU split + SolidSyslogPoolAllocator. Mirrors S11.06's GetAddrInfoResolver migration: the class is truly stateless, but the pool is kept for lifecycle symmetry with FreeRtosStaticResolver (S11.08) which carries IPv4 octet state. Mixed _Get/_Create shape within the Resolver base class would force integrators to remember which sibling wants which lifecycle; uniform pool semantics wins. The two file-scope Winsock_getaddrinfo / Winsock_freeaddrinfo function- pointer test seams stay in WinsockResolver.c per the C4232 forwarder workaround. The Internal.h header continues to expose them to WinsockFake. Public _Destroy(void) → _Destroy(base). SOLIDSYSLOG_WINSOCK_RESOLVER_POOL_SIZE default 1U. Pool exhaustion returns shared SolidSyslogNullResolver with ERROR; unknown/stale destroy emits WARNING. BddTargetWindows caller updated; test file rewires teardown and gains a Pool TEST_GROUP of 9 tests mirroring SolidSyslogGetAddrInfoResolverPool.
Canonical 3-TU split + SolidSyslogPoolAllocator, mirroring S11.06's PosixFile migration. The DEFAULT_INSTANCE / DESTROYED_INSTANCE static const structs (the storage-cast variant's vtable-and-Fd seed values) are gone — Initialise wires the vtable + sets Fd = INVALID_FD explicitly; Cleanup overwrites the base with the shared NullFile vtable. Public _Create(void) shape; _Destroy keeps its base-handle parameter. SolidSyslogWindowsFileStorage typedef and SOLIDSYSLOG_WINDOWS_FILE_SIZE enum gone from the public header. SOLIDSYSLOG_WINDOWS_FILE_POOL_SIZE default 1U. Pool exhaustion returns shared SolidSyslogNullFile with ERROR; unknown/stale destroy emits WARNING. BddTargetWindows caller drops the local fileStorage; test file drops the storage local and the CreateReturnsHandleInsideCallerSuppliedStorage test (no longer meaningful when the slot is library-internal) and gains a Pool TEST_GROUP of 9 tests mirroring SolidSyslogPosixFilePool.
Canonical 3-TU split + SolidSyslogPoolAllocator. Refactor-only: every behaviour preserved verbatim — the non-blocking connect with select() timeout, keepalive setsockopt cluster, TCP_NODELAY, ioctlsocket FIONBIO, WSAEWOULDBLOCK handling in Read, fail-fast Send-then-Close. All ten WinsockTcpStream_* function-pointer test seams stay in WinsockTcpStream.c per the C4232 __declspec(dllimport) workaround. WinsockTcpStream_Initialise explicitly sets Fd = INVALID_SOCKET to avoid pool zero-init giving a valid SOCKET handle (Windows SOCKET 0 is not INVALID_SOCKET, matching the PosixDatagram / WinsockDatagram lesson). The DEFAULT_INSTANCE / DESTROYED_INSTANCE static const structs are gone; Cleanup calls Close then overwrites the base with the shared NullStream vtable. Public _Create(void) shape; _Destroy keeps its base-handle parameter. SolidSyslogWinsockTcpStreamStorage and SOLIDSYSLOG_WINSOCK_TCP_STREAM_SIZE gone from the public header. SOLIDSYSLOG_WINSOCK_TCP_STREAM_POOL_SIZE default 2U — matches the POSIX sibling so the BDD target's plain-TCP + TLS-underlying-TCP pair share one pool. Pool exhaustion returns shared SolidSyslogNullStream with ERROR; unknown/stale destroy emits WARNING. mstcpip.h now explicitly preceded by an explicit winsock2.h include (the prior include order worked by accident — Internal.h transitively pulled in winsock2.h before mstcpip.h; the new TU layout exposes the ordering and makes it explicit). BddTargetWindows + BddTargetTlsSender_OpenSsl_WinsockTcp callers updated (both drop their *Storage locals). Test file drops the storage local and the CreateReturnsHandleInsideCallerSuppliedStorage test, and gains a Pool TEST_GROUP of 9 tests mirroring SolidSyslogPosixTcpStreamPool.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates five Windows adapter classes (Mutex, File, Winsock TCP Stream, Datagram, and Resolver) from storage-based lifecycle APIs to pool-backed allocation patterns. Each adapter follows a consistent 3-TU split with pool-size tunables, no-argument creation and pointer-based destruction, new private headers with internal struct definitions, and comprehensive error handling for pool exhaustion and unknown destroy requests. ChangesWindows adapters onto PoolAllocator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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)
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 |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1263 passed, 🙈 2 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Platform/Windows/Source/SolidSyslogWinsockResolverStatic.c (1)
16-18: ⚡ Quick winAlign helper declarations/definitions with the source helper placement rule.
WinsockResolver_IndexFromHandleandWinsockResolver_CleanupAtIndexare declared asstaticand defined at file end. The project rule asks for helper functions in.cfiles to bestatic inlineand placed immediately beneath the first caller.As per coding guidelines,
**/*.c: Forward-declare helper functions at the top of source files (after constants/types) as static inline and define immediately beneath the function that first calls them.Also applies to: 54-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Platform/Windows/Source/SolidSyslogWinsockResolverStatic.c` around lines 16 - 18, The two helper functions WinsockResolver_IndexFromHandle and WinsockResolver_CleanupAtIndex are currently declared static and defined at the end of the file; change them to static inline, move their full definitions to immediately beneath the first function that calls them, and remove the trailing forward declarations at the file end so the helper placement follows the project's rule; apply the same change for the other helper declarations identified (the ones referenced around lines 54-72) so all helper functions are declared static inline and defined immediately after their first caller (update any callers to rely on the new inline definitions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Bdd/Targets/Windows/BddTargetWindows.c`:
- Around line 222-223: The DestroySender function references an undeclared local
variable resolver; move the resolver handle to file-scope like udpDatagram and
plainTcpStream so DestroySender can access it. Declare a file-scope variable
(e.g., static SolidSyslogWinsockResolver* resolver;), update CreateSender to
assign to that file-scope variable instead of creating a new local resolver, and
ensure DestroySender calls SolidSyslogWinsockResolver_Destroy(resolver) and
clears the file-scope variable during teardown.
---
Nitpick comments:
In `@Platform/Windows/Source/SolidSyslogWinsockResolverStatic.c`:
- Around line 16-18: The two helper functions WinsockResolver_IndexFromHandle
and WinsockResolver_CleanupAtIndex are currently declared static and defined at
the end of the file; change them to static inline, move their full definitions
to immediately beneath the first function that calls them, and remove the
trailing forward declarations at the file end so the helper placement follows
the project's rule; apply the same change for the other helper declarations
identified (the ones referenced around lines 54-72) so all helper functions are
declared static inline and defined immediately after their first caller (update
any callers to rely on the new inline definitions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7db4ef8-22d9-49ce-9b43-ce4425c69e2e
📒 Files selected for processing (32)
Bdd/Targets/Common/BddTargetTlsSender_OpenSsl_WinsockTcp.cBdd/Targets/Windows/BddTargetWindows.cCLAUDE.mdCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/SolidSyslogErrorMessages.hDEVLOG.mdPlatform/Windows/CMakeLists.txtPlatform/Windows/Interface/SolidSyslogWindowsFile.hPlatform/Windows/Interface/SolidSyslogWindowsMutex.hPlatform/Windows/Interface/SolidSyslogWinsockDatagram.hPlatform/Windows/Interface/SolidSyslogWinsockResolver.hPlatform/Windows/Interface/SolidSyslogWinsockTcpStream.hPlatform/Windows/Source/SolidSyslogWindowsFile.cPlatform/Windows/Source/SolidSyslogWindowsFilePrivate.hPlatform/Windows/Source/SolidSyslogWindowsFileStatic.cPlatform/Windows/Source/SolidSyslogWindowsMutex.cPlatform/Windows/Source/SolidSyslogWindowsMutexPrivate.hPlatform/Windows/Source/SolidSyslogWindowsMutexStatic.cPlatform/Windows/Source/SolidSyslogWinsockDatagram.cPlatform/Windows/Source/SolidSyslogWinsockDatagramPrivate.hPlatform/Windows/Source/SolidSyslogWinsockDatagramStatic.cPlatform/Windows/Source/SolidSyslogWinsockResolver.cPlatform/Windows/Source/SolidSyslogWinsockResolverPrivate.hPlatform/Windows/Source/SolidSyslogWinsockResolverStatic.cPlatform/Windows/Source/SolidSyslogWinsockTcpStream.cPlatform/Windows/Source/SolidSyslogWinsockTcpStreamPrivate.hPlatform/Windows/Source/SolidSyslogWinsockTcpStreamStatic.cTests/SolidSyslogWindowsFileTest.cppTests/SolidSyslogWindowsMutexTest.cppTests/SolidSyslogWinsockDatagramTest.cppTests/SolidSyslogWinsockResolverTest.cppTests/SolidSyslogWinsockTcpStreamTest.cpp
WinsockResolver_Destroy now needs the handle, but the previous void-arg shape let resolver live as a CreateSender local. DestroySender doesn't see that local — the build failed on CI's MSVC job at BddTargetWindows.c(223,40): 'resolver': undeclared identifier. Match the file-scope-static pattern the surrounding code already uses for udpDatagram, plainTcpStream, and the senders. Belongs logically in the S11.07 WinsockResolver commit; landing as a follow-up because the original was already pushed. Build verified with --target SolidSyslogBddTarget locally (missed in the original commit — only SolidSyslogTests was being built per commit; default-target build catches this).
CodeRabbit findings — dispositionBdd/Targets/Windows/BddTargetWindows.c:223 (Critical — Confirmed. Fixed in SolidSyslogWinsockResolverStatic.c:16-18 (Nitpick — helpers should be The CLAUDE.md rule is correct, but the established Fixing only S11.07 would create cross-class inconsistency with the eleven already-merged |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1263 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Second platform-adapter sweep in E11 (#29). Applies the canonical 3-TU split +
SolidSyslogPoolAllocatorto every stateful Windows adapter class:WindowsMutex,WindowsFile,WinsockTcpStream(storage-cast), andWinsockDatagram,WinsockResolver(file-scope singleton).S11.07 is a pure migration sweep — no new public GoF nulls. The four needed (
NullStream/NullDatagram/NullFile/NullResolver) all landed in S11.06;NullMutexalready migrated to_Get(void)there too.Closes #406.
Change Description
Five per-class migration commits + a DEVLOG entry. Each per-class migration:
<Class>Storagetypedef andSOLIDSYSLOG_*_SIZEenum (where applicable)_Createto takevoid(where applicable)_Destroy(base)shape for the previously_Destroy(void)singletonsPrivate.h/Static.cper the S11.04/S11.06 precedentPoolTEST_GROUP of 9 tests mirroring the POSIX siblingsThree decisions worth surfacing:
WinsockResolverpool-migrated, not_Get. It's truly stateless today, butFreeRtosStaticResolver(S11.08) carries IPv4 octet state and needs the pool. Mixed_Get/_Createwithin the Resolver base class would force integrators to remember which sibling wants which lifecycle. Same precedent as S11.06'sGetAddrInfoResolver.WinsockDatagram_InitialiseandWinsockTcpStream_Initialiseexplicitly setFd = INVALID_SOCKET. Pool slots are zero-initialised butSOCKET 0is a valid handle on Windows, not invalid. Same root cause as S11.06's PosixDatagram fix.WindowsMutex_Initialisehas no partial-init guard.InitializeCriticalSectionisvoidand the pre-migration code didn't handle failure; refactor-only preserves that. UnlikePosixMutex_Initialisewhich guardspthread_mutex_init's return.The ten
WinsockTcpStream_*and sixWinsock_*(Datagram) and twoWinsock_*(Resolver) function-pointer test seams stay in the per-class.cfiles per the C4232__declspec(dllimport)forwarder workaround. The correspondingInternal.hheaders continue to expose them to WinsockFake.One incidental fix in WinsockTcpStream:
mstcpip.hwas being parsed correctly only becauseInternal.htransitively pulled inwinsock2.hfirst. The new TU layout makes that ordering explicit via a documented#include <winsock2.h>ahead of<mstcpip.h>.Verification
msvc-debugpreset (local): full 1131-test suite green; per-class Pool TEST_GROUPs add 45 tests (9 × 5 classes).tidypreset clean.iwyutarget clean — 1060 files audited, 0 actionable findings.BddTargetTlsSender_OpenSsl_WinsockTcpupdated for the new API.feedback_devlog_in_pr).Out of scope
Per the E11 epic sequence:
WindowsAtomicCounterdefers to S11.09 ("AtomicCounter family + TLS + FatFs") so it migrates alongsideStdAtomicCounter.SolidSyslogAddress(Windows variant) is a utility struct, no Create/Destroy. FreeRTOS adapters land in S11.08.Summary by CodeRabbit
Breaking Changes
New Features
Documentation