feat!: S24.07 SolidSyslogAddress as pool-allocated handle#422
Merged
Conversation
First slice of S24.07. Adds the POSIX platform Address as a pool- allocated handle wrapping struct sockaddr_in, sized by the new SOLIDSYSLOG_ADDRESS_POOL_SIZE tunable (default 3 to fit the canonical BDD UDP + plain-TCP + TLS-stream wiring without an override). Pool exhaustion falls back to a TU-private singleton sized as a real SolidSyslogPosixAddress, so a Resolver overwrite into the fallback is bounded. A cross-platform Core/-side NullAddress would have to be either platform-blind (and undersized — UB on Resolver write) or platform-aware (and break the layering); the per-platform TU-private fallback avoids both. Multi-overflow integrators share the fallback storage and race; the POOL_EXHAUSTED error handler call at every exhaustion is the signal to bump the tunable. Pool plumbing duplicates the established 3-TU split (Class.c + ClassPrivate.h + ClassStatic.c) used by every other E11 pool-class, with the standard pool / lock / error / stale-destroy tests. AsSockaddrIn / AsConstSockaddrIn accessors are TU-private — exposed only via the Private.h header that library internals (Resolver / Datagram / Stream) will include in later slices. No callers yet — additive only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review fixes on the PosixAddress slice: - SOLIDSYSLOG_ADDRESS_POOL_SIZE defaults to 1; BDD-target wirings override via SOLIDSYSLOG_USER_TUNABLES_FILE in their respective build harnesses. - Pool-slot impl-to-base cast moves into a PosixAddress_HandleFromIndex helper; the three call sites (Create success, IndexFromHandle, CleanupAtIndex) now route through one place. Fallback cast stays inline — it points to a separate static, one occurrence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Duplicate-and-rename of the PosixAddress slice for the Winsock target. Same 3-TU split, same TU-private fallback shape, same set of pool plumbing tests. Storage struct wraps the Winsock sockaddr_in from <winsock2.h>; AsSockaddrIn / AsConstSockaddrIn accessors are TU-private via SolidSyslogWinsockAddressPrivate.h. Reuses the shared SOLIDSYSLOG_ADDRESS_POOL_SIZE tunable — one pool size across all three platforms. Verified locally on the cpputest gcc image (Posix-side build green, new Winsock sources excluded from the link as expected). Windows build verification deferred to CI (build-windows-msvc, integration-windows-openssl, bdd-windows-otel). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Duplicate-and-rename of the PosixAddress slice for the FreeRTOS target. Same 3-TU split, same TU-private fallback shape, same set of pool plumbing tests. Storage struct wraps struct freertos_sockaddr from <FreeRTOS_Sockets.h>; AsFreertosSockaddr / AsConstFreertosSockaddr accessors are TU-private via SolidSyslogFreeRtosAddressPrivate.h. Reuses the shared SOLIDSYSLOG_ADDRESS_POOL_SIZE tunable. Verified locally in the cpputest-freertos host image: SolidSyslogFreeRtosAddressTest 11/11 green; the four other FreeRTOS host tests (Datagram, Mutex, StaticResolver, SysUpTime, TcpStream) all still green; SolidSyslogTests 1291/1293; MbedTlsStream tests 42/42. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches the canonical BDD multi-transport wiring (one Address per Sender across UDP + plain-TCP + TLS-stream). Same trade-off applied by SOLIDSYSLOG_POSIX_TCP_STREAM_POOL_SIZE and SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE — single-transport integrators pay a small unused-slot cost, multi-transport integrators get the canonical wiring without an override. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a required Address field to SolidSyslogUdpSenderConfig and drops the inline 128-byte SolidSyslogAddressStorage that every UdpSender slot carried under the old caller-supplied-storage pattern. UdpSender now reads the destination slot via self->Config.Address; the platform Address pool owns the storage. Bad-setup contract: Create rejects a NULL Address handle with SOLIDSYSLOG_ERROR_MSG_UDPSENDER_CREATE_NULL_ADDRESS and the resulting sender resolves to SolidSyslogNullSender, matching the existing NULL-Resolver / NULL-Datagram / NULL-Endpoint shape. Threads the new field through every UdpSender consumer: SolidSyslogUdpSenderTest (and the Retry / Reconnect / Endpoint test groups it backs), BddTargetServiceThreadTest, and the three BDD-target main.c files (Linux / Windows / FreeRtos) — each wires an Address handle from its platform's pool and destroys it after the Sender. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a required Address field to SolidSyslogStreamSenderConfig and drops the per-Connect stack-allocated SolidSyslogAddressStorage that StreamSender_Connect used to set up its resolution target. The platform Address pool now owns the storage; StreamSender_Connect just reads self->Config.Address directly. Threads the new field through every StreamSender consumer: SolidSyslogStreamSenderTest (five TEST_GROUPs, each wiring its own PosixAddress), the three BDD-target main.c files (Linux PosixAddress + Windows WinsockAddress + FreeRtos FreeRtosAddress for their plain- TCP StreamSenders), and the three TLS-sender glue files in Bdd/Targets/Common/ (each TLS StreamSender now creates and owns its own platform Address — one per Sender, no sharing). Each BDD wiring now consumes three Address pool slots (UDP + plain-TCP + TLS), exactly fitting the SOLIDSYSLOG_ADDRESS_POOL_SIZE=3 default. StreamSender keeps its existing permissive bad-setup posture (no NULL-field validation) — tightening it is out of scope for S24.07. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Library-internal consumers of the platform sockaddr (Resolver,
Datagram, Stream impls on POSIX / Winsock / FreeRTOS) move from the
old shared SolidSyslogAddressInternal.h accessor names
(SolidSyslogAddress_AsSockaddrIn / _AsConstSockaddrIn /
_AsFreertosSockaddr / _AsConstFreertosSockaddr) onto the
platform-specific accessors that live alongside each platform pool
class (SolidSyslogPosixAddress_AsSockaddrIn etc., declared in
SolidSyslog{Posix,Winsock,FreeRtos}AddressPrivate.h).
11 files migrated, one site per file. The three legacy
SolidSyslogAddressInternal.h headers are now unreferenced — they
get deleted alongside the rest of the legacy Address surface in
the closing slice.
Verified locally in the cpputest-freertos host image:
SolidSyslogTests 1292/1294; BddTargetTests 48/48; all five FreeRTOS
host tests green (Address 11, Datagram 30, Mutex 13, StaticResolver
18, SysUpTime 4, TcpStream 46).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes out S24.07. Removes the caller-supplied SolidSyslogAddressStorage
API now that every site has been migrated onto the per-platform pool
classes:
- Slims Core/Interface/SolidSyslogAddress.h to the opaque forward
declaration only — SolidSyslogAddressStorage, SOLIDSYSLOG_ADDRESS_SIZE,
and SolidSyslogAddress_FromStorage are gone.
- Deletes the three identical Platform/{Posix,Windows,FreeRtos}/Source/
SolidSyslogAddress.c factory files plus their CMake entries.
- Deletes the three Platform/{Posix,Windows,FreeRtos}/Source/
SolidSyslogAddressInternal.h headers (the AsSockaddrIn / AsFreertosSockaddr
accessors moved to SolidSyslog{Posix,Winsock,FreeRtos}AddressPrivate.h
in the previous slice).
- Deletes Tests/SolidSyslogAddressTest.cpp — the only test it carried
was for _FromStorage, which no longer exists. Pool-class behaviour
is covered by the three new per-platform Address tests.
- Migrates 13 test fixtures off SolidSyslogAddressStorage onto
SolidSyslog{Posix,Winsock,FreeRtos}Address_Create — covers the
POSIX / Winsock / FreeRTOS datagram + stream + resolver tests, the
TLS unit + integration tests on both the OpenSSL and Mbed TLS
reference adapters, and StreamFakeTest.
- Adds Platform/FreeRtos/Source to the three FreeRTOS host test
executables' include paths so they can reach the new
SolidSyslogFreeRtosAddressPrivate.h accessors.
- Compiles SolidSyslogFreeRtosAddress{.c,Static.c} into the
FreeRtosDatagramTest / FreeRtosTcpStreamTest / FreeRtosStaticResolverTest
executables (they don't link libSolidSyslog at FREERTOS_PATH-build
time).
Verified locally in the cpputest-freertos host image:
SolidSyslogTests 1290/1292; BddTargetTests 48/48; all six FreeRTOS
host tests green; MbedTlsStreamTest 42/42; OpenSslIntegrationTests
9/9; MbedTlsIntegrationTests 7/7.
BREAKING CHANGE: SolidSyslogAddressStorage and SolidSyslogAddress_FromStorage
are removed. Integrators must allocate destination addresses through the
platform pool — SolidSyslog{Posix,Winsock,FreeRtos}Address_Create — and
wire the returned handle into UdpSenderConfig.Address /
StreamSenderConfig.Address.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uppressions + DEVLOG
Closes out S24.07's documentation tail:
- docs/misra-deviations.md D.002: drops the (b) Address paragraph
(caller-supplied-storage rationale retired); folds Address into
D.002(a) standard vtable / opaque-handle downcast scope.
- misra_suppressions.txt: removes the nine legacy
SolidSyslogAddress.c / SolidSyslogAddressInternal.h suppression
entries; adds the new SolidSyslog{Posix,Winsock,FreeRtos}Address
(`.c` + `Private.h` + `Static.c`) cast sites under the same D.002
scope.
- CLAUDE.md public-header table: adds rows for `SolidSyslogAddress.h`
(opaque only), `SolidSyslog{Posix,Winsock,FreeRtos}Address.h`; updates
`SolidSyslog{Udp,Stream}Sender.h` to mention the new `Address` field.
- Inline cppcheck-suppress cstyleCast in each per-platform
AddressPrivate.h — opaque-to-impl downcast, explained at the site
(cstyleCast is a non-MISRA cppcheck style warning; the MISRA-rule
suppressions for the same casts live in misra_suppressions.txt).
- DEVLOG entry documenting the per-platform-fallback design pivot,
pool default 3, HandleFromIndex helper, and the deferred
StreamSender bad-setup contract alignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add direct <netinet/in.h> to SolidSyslogPosixAddress.c (was relying on transitive include from Private.h). - Trim per-file unused includes (<string.h>, <cstdint>) and add <arpa/inet.h> / <sys/socket.h> where IWYU pointed out missing direct includes for htons / inet_pton / AF_INET in tests touched by the S24.07 fixture migrations. - Make SolidSyslogGetAddrInfoResolverTest::Resolve const (readability-make-member-function-const). - clang-format pass on every file the PR touches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR migrates SolidSyslogAddress from caller-owned storage to per-platform pool-allocated opaque handles. Sender configs now require an Address pointer; platform-specific Posix/Winsock/FreeRTOS pools provide Create/Destroy with fallback and tests/build/docs updated accordingly. ChangesAddress pooling refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant AddressPool as Address Pool
participant Sender
participant Resolver
participant Network
Client->>AddressPool: SolidSyslogPosixAddress_Create()
AddressPool-->>Client: address handle
Client->>Sender: SolidSyslogUdpSender_Create(config { Address: address, ... })
Sender->>Sender: store config->Address
Sender->>Resolver: Resolve(transport, host, port, address)
Resolver->>AddressPool: SolidSyslogPosixAddress_AsSockaddrIn(address)
Resolver-->>Sender: resolved
Sender->>Network: sendto using sockaddr_in from address
Client->>Sender: SolidSyslogUdpSender_Destroy(sender)
Client->>AddressPool: SolidSyslogPosixAddress_Destroy(address)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1296 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp (1)
165-173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace layout-dependent address reinterpret_cast in TCP connect assertion
- In
Tests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp(lines 165-173), the connect address check uses a layout-dependentreinterpret_cast. UseSolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr)(already used in the datagram test) to assert the address handle via the accessor.♻️ Suggested update
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) -- platform-layout cast, see setup - POINTERS_EQUAL(reinterpret_cast<const struct freertos_sockaddr*>(addr), FreeRtosSocketsFake_LastConnectAddress()); + POINTERS_EQUAL(SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr), FreeRtosSocketsFake_LastConnectAddress());🤖 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 `@Tests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp` around lines 165 - 173, The test uses a layout-dependent reinterpret_cast to check the connect address; change the assertion to use the accessor SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr) instead of reinterpret_cast so the test is portable — update the POINTERS_EQUAL call in TEST(SolidSyslogFreeRtosTcpStream, OpenCallsConnectWithSocketAndAddress) to compare SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr) with FreeRtosSocketsFake_LastConnectAddress(), keeping the other checks (socket equality via FreeRtosSocketsFake_LastSocketReturned()/LastConnectSocket and address length assertion) intact.
🧹 Nitpick comments (1)
Platform/Posix/Source/SolidSyslogPosixAddressStatic.c (1)
58-81: ⚡ Quick winPlace helper definitions directly beneath their first caller.
PosixAddress_HandleFromIndex,PosixAddress_IndexFromHandle, andPosixAddress_CleanupAtIndexare currently grouped at file end; this file-level rule expects helper definitions immediately below first use.As per coding guidelines "
**/*.c: Within source files, order functions top-down: _Create first, _Destroy second, public functions in API order, helpers forward-declared and defined immediately beneath their first caller".🤖 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/Posix/Source/SolidSyslogPosixAddressStatic.c` around lines 58 - 81, Move the helper functions PosixAddress_HandleFromIndex, PosixAddress_IndexFromHandle, and PosixAddress_CleanupAtIndex so they are defined immediately below their first caller (or alternatively add minimal forward declarations and place their full definitions right after the first caller) to satisfy the source-file ordering rule; locate where the first use of these helpers occurs and relocate the full definitions there (updating any includes/forward-decls if necessary) so helpers are no longer grouped at the file end.
🤖 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 `@CLAUDE.md`:
- Around line 327-329: Pool-exhaustion currently returns a shared mutable
fallback (PosixAddress_Fallback / WinsockAddress_Fallback /
FreeRtosAddress_Fallback) that resolvers (Platform/.../SolidSyslog*Resolver.c)
write into, causing races; change the fallback to a safe immutable null object
and return that instead. Implement a SolidSyslogNullAddress (symbol:
SolidSyslogNullAddress and accessors matching SolidSyslogAddress API), update
SolidSyslogPosixAddress_Create, SolidSyslogWinsockAddress_Create,
SolidSyslogFreeRtosAddress_Create to return the null address on pool exhaustion
(remove returning the shared mutable *_Fallback), and update resolvers (e.g.,
SolidSyslogGetAddrInfoResolver.c, SolidSyslogWinsockResolver.c,
SolidSyslogFreeRtosStaticResolver.c) to detect the null address (or use
defensive accessors) so they never write into shared fallback storage. Ensure no
shared mutable *_Fallback is exposed to callers and that the null address is
immutable/safe for concurrent use.
In `@Core/Source/SolidSyslogStreamSender.c`:
- Line 55: StreamSender_Initialise and downstream code assume config and
config->Address are non-NULL which can crash; add create-time validation in
SolidSyslogStreamSender_Create to check that config and config->Address are not
NULL, mirror the pattern used by SolidSyslogUdpSender_Create (call
UdpSender_IsValidConfig or equivalent), and if invalid log
SOLIDSYSLOG_ERROR_MSG_UDPSENDER_CREATE_NULL_ADDRESS (or a stream-specific error
constant) and return a failure/NULL before calling StreamSender_Initialise or
StreamSender_Connect so that SolidSyslogResolver_Resolve never receives a NULL
addr.
In `@Tests/StreamFakeTest.cpp`:
- Line 1: The test file StreamFakeTest.cpp unconditionally depends on POSIX-only
symbols (SolidSyslogPosixAddress.h and functions SolidSyslogPosixAddress_Create
/ SolidSyslogPosixAddress_Destroy); guard the inclusion and usages with `#ifdef`
SOLIDSYSLOG_POSIX (or provide a small non-POSIX stub) so non-POSIX builds
compile, and update Tests/CMakeLists.txt to only add StreamFakeTest.cpp when
SOLIDSYSLOG_POSIX is ON (or add a platform-selected test helper target) to avoid
linking POSIX-only platform code into non-POSIX test builds.
---
Outside diff comments:
In `@Tests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp`:
- Around line 165-173: The test uses a layout-dependent reinterpret_cast to
check the connect address; change the assertion to use the accessor
SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr) instead of
reinterpret_cast so the test is portable — update the POINTERS_EQUAL call in
TEST(SolidSyslogFreeRtosTcpStream, OpenCallsConnectWithSocketAndAddress) to
compare SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr) with
FreeRtosSocketsFake_LastConnectAddress(), keeping the other checks (socket
equality via FreeRtosSocketsFake_LastSocketReturned()/LastConnectSocket and
address length assertion) intact.
---
Nitpick comments:
In `@Platform/Posix/Source/SolidSyslogPosixAddressStatic.c`:
- Around line 58-81: Move the helper functions PosixAddress_HandleFromIndex,
PosixAddress_IndexFromHandle, and PosixAddress_CleanupAtIndex so they are
defined immediately below their first caller (or alternatively add minimal
forward declarations and place their full definitions right after the first
caller) to satisfy the source-file ordering rule; locate where the first use of
these helpers occurs and relocate the full definitions there (updating any
includes/forward-decls if necessary) so helpers are no longer grouped at the
file end.
🪄 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: a7aa2ba5-1f2f-4821-ba65-ba7a856b64b8
📒 Files selected for processing (71)
Bdd/Targets/Common/BddTargetTlsSender_MbedTls_FreeRtosTcp.cBdd/Targets/Common/BddTargetTlsSender_OpenSsl_PosixTcp.cBdd/Targets/Common/BddTargetTlsSender_OpenSsl_WinsockTcp.cBdd/Targets/FreeRtos/main.cBdd/Targets/Linux/main.cBdd/Targets/Windows/BddTargetWindows.cCLAUDE.mdCore/Interface/SolidSyslogAddress.hCore/Interface/SolidSyslogStreamSender.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Interface/SolidSyslogUdpSender.hCore/Source/SolidSyslogErrorMessages.hCore/Source/SolidSyslogStreamSender.cCore/Source/SolidSyslogUdpSender.cCore/Source/SolidSyslogUdpSenderPrivate.hCore/Source/SolidSyslogUdpSenderStatic.cDEVLOG.mdPlatform/FreeRtos/Interface/SolidSyslogFreeRtosAddress.hPlatform/FreeRtos/Source/SolidSyslogAddress.cPlatform/FreeRtos/Source/SolidSyslogAddressInternal.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosAddress.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosAddressPrivate.hPlatform/FreeRtos/Source/SolidSyslogFreeRtosAddressStatic.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.cPlatform/Posix/CMakeLists.txtPlatform/Posix/Interface/SolidSyslogPosixAddress.hPlatform/Posix/Source/SolidSyslogAddress.cPlatform/Posix/Source/SolidSyslogAddressInternal.hPlatform/Posix/Source/SolidSyslogGetAddrInfoResolver.cPlatform/Posix/Source/SolidSyslogPosixAddress.cPlatform/Posix/Source/SolidSyslogPosixAddressPrivate.hPlatform/Posix/Source/SolidSyslogPosixAddressStatic.cPlatform/Posix/Source/SolidSyslogPosixDatagram.cPlatform/Posix/Source/SolidSyslogPosixTcpStream.cPlatform/Windows/CMakeLists.txtPlatform/Windows/Interface/SolidSyslogWinsockAddress.hPlatform/Windows/Source/SolidSyslogAddress.cPlatform/Windows/Source/SolidSyslogAddressInternal.hPlatform/Windows/Source/SolidSyslogWinsockAddress.cPlatform/Windows/Source/SolidSyslogWinsockAddressPrivate.hPlatform/Windows/Source/SolidSyslogWinsockAddressStatic.cPlatform/Windows/Source/SolidSyslogWinsockDatagram.cPlatform/Windows/Source/SolidSyslogWinsockResolver.cPlatform/Windows/Source/SolidSyslogWinsockTcpStream.cTests/Bdd/Targets/BddTargetServiceThreadTest.cppTests/CMakeLists.txtTests/FreeRtos/CMakeLists.txtTests/FreeRtos/SolidSyslogFreeRtosAddressTest.cppTests/FreeRtos/SolidSyslogFreeRtosDatagramTest.cppTests/FreeRtos/SolidSyslogFreeRtosStaticResolverTest.cppTests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cppTests/MbedTls/SolidSyslogMbedTlsStreamTest.cppTests/MbedTlsIntegration/SolidSyslogMbedTlsStreamIntegrationTest.cppTests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cppTests/SolidSyslogAddressTest.cppTests/SolidSyslogGetAddrInfoResolverTest.cppTests/SolidSyslogPosixAddressTest.cppTests/SolidSyslogPosixDatagramTest.cppTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogStreamSenderTest.cppTests/SolidSyslogTlsStreamTest.cppTests/SolidSyslogUdpSenderTest.cppTests/SolidSyslogWinsockAddressTest.cppTests/SolidSyslogWinsockDatagramTest.cppTests/SolidSyslogWinsockResolverTest.cppTests/SolidSyslogWinsockTcpStreamTest.cppTests/StreamFakeTest.cppdocs/misra-deviations.mdmisra_suppressions.txt
💤 Files with no reviewable changes (9)
- Core/Source/SolidSyslogUdpSenderPrivate.h
- Platform/Windows/Source/SolidSyslogAddress.c
- Platform/Posix/Source/SolidSyslogAddressInternal.h
- Platform/Posix/Source/SolidSyslogAddress.c
- Tests/SolidSyslogAddressTest.cpp
- Platform/FreeRtos/Source/SolidSyslogAddress.c
- Platform/Windows/Source/SolidSyslogAddressInternal.h
- Platform/FreeRtos/Source/SolidSyslogAddressInternal.h
- Core/Interface/SolidSyslogAddress.h
Comment on lines
+327
to
+329
| | `SolidSyslogPosixAddress.h` | System setup code on POSIX targets wiring a UDP / TCP destination | `SolidSyslogPosixAddress_Create(void)`, `_Destroy(base)` (wraps `struct sockaddr_in`). Instance struct lives in a library-internal static pool sized by `SOLIDSYSLOG_ADDRESS_POOL_SIZE` (default 3 — covers the canonical multi-transport BDD wiring). Pool-exhaustion fallback is a TU-private singleton sized as a real `SolidSyslogPosixAddress`; multi-overflow integrators share that storage and race. | | ||
| | `SolidSyslogWinsockAddress.h` | System setup code on Windows targets wiring a UDP / TCP destination | `SolidSyslogWinsockAddress_Create(void)`, `_Destroy(base)` (wraps Winsock `struct sockaddr_in`). Same pool semantics as `SolidSyslogPosixAddress`. | | ||
| | `SolidSyslogFreeRtosAddress.h` | System setup code on FreeRTOS targets wiring a UDP / TCP destination | `SolidSyslogFreeRtosAddress_Create(void)`, `_Destroy(base)` (wraps `struct freertos_sockaddr`). Same pool semantics as `SolidSyslogPosixAddress`. | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for NullAddress implementation across the codebase
rg -n --type=c --type=h -C3 'SolidSyslogNullAddress'
# Check for TU-private singleton patterns in platform address implementations
rg -nP --type=c 'static\s+(struct\s+)?SolidSyslog(Posix|Winsock|FreeRtos)Address\s+\w*(Null|Fallback|Singleton)' \
Platform/Repository: DavidCozens/solid-syslog
Length of output: 442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the fallback singletons are used/assigned
rg -n --type=c --type=h 'WinsockAddress_Fallback|FreeRtosAddress_Fallback|PosixAddress_Fallback' Platform -S -g'*.c' -g'*.h'
# Show the fallback/static implementations to see if they're mutable and how they're populated
for f in Platform/Windows/Source/SolidSyslogWinsockAddressStatic.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosAddressStatic.c \
Platform/Posix/Source/SolidSyslogPosixAddressStatic.c; do
echo "---- $f ----"
nl -ba "$f" | sed -n '1,220p'
done
# Search for any "NullAddress" / "Null*Address" symbol variants
rg -n --type=c --type=h --hidden 'Null.*Address|SolidSyslogNullAddress|SolidSyslogNull.*Address' .
# Look for the pool exhaustion / overflow handling path for addresses
rg -n --type=c 'pool|POOL|exhaust|overflow|fallback|Address_.*Fallback|_Fallback' Platform -S -g'*.c'Repository: DavidCozens/solid-syslog
Length of output: 1037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
Platform/Posix/Source/SolidSyslogPosixAddressStatic.c
Platform/Windows/Source/SolidSyslogWinsockAddressStatic.c
Platform/FreeRtos/Source/SolidSyslogFreeRtosAddressStatic.c
)
for f in "${files[@]}"; do
echo "## $f (lines: $(wc -l < "$f"))"
done
echo
for f in "${files[@]}"; do
echo "---- $f (top 220 lines) ----"
sed -n '1,220p' "$f"
done
echo
echo "## Usage of fallback singletons"
rg -n --type=c --type=h 'PosixAddress_Fallback|WinsockAddress_Fallback|FreeRtosAddress_Fallback' Platform -S
echo
echo "## Any NullAddress-like symbols"
rg -n --type=c --type=h --hidden 'SolidSyslogNullAddress|Null.*Address' .
echo
echo "## Address pool exhaustion/overflow handling (platform-specific)"
rg -n --type=c 'exhaust|overflow|pool|fallback' Platform -S -g'*.c'Repository: DavidCozens/solid-syslog
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all usages of the fallback address objects (should be minimal: create + write-through sites)
rg -n --type=c --type=h 'PosixAddress_Fallback|WinsockAddress_Fallback|FreeRtosAddress_Fallback' Platform -S
# 2) Inspect how platform address structs store state and whether they are written by resolvers
rg -n --type=c --type=h 'PosixAddress_Initialise|WinsockAddress_Initialise|FreeRtosAddress_Initialise' Platform -S
rg -n --type=c --type=h 'PosixAddress_Cleanup|WinsockAddress_Cleanup|FreeRtosAddress_Cleanup' Platform -S
# 3) Find resolver implementations and see how they write into the provided address
rg -n --type=c --type=h 'Resolver.*(Address|addr)' Platform -S
rg -n --type=c --type=h '(GetAddrInfo|WinsockResolver).*Open|.*Resolve|.*Update|.*Set' Platform -S
# 4) Pull small targeted excerpts for resolver functions and address write paths.
# (We can't dump huge files, so extract just the functions around resolver->address usage.)
files_to_snip=(
Platform/Posix/Source/SolidSyslogGetAddrInfoResolverStatic.c
Platform/Windows/Source/SolidSyslogWinsockResolverStatic.c
)
for f in "${files_to_snip[@]}"; do
if [ -f "$f" ]; then
echo "## Snip: $f"
# print first 120 lines and any lines matching Address/addr inside the file
sed -n '1,140p' "$f"
echo "## Matching lines (Address/addr) in $f"
rg -n 'Address|addr' "$f" -S | head -n 80
fi
done
# 5) Locate the actual address "setter/field write" functions used by resolvers.
rg -n --type=c --type=h 'sockaddr_in|freertos_sockaddr|memcpy|strcpy|Assign|Set.*(addr|Addr|address)' Platform -SRepository: DavidCozens/solid-syslog
Length of output: 32598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Address initialise implementations (what they write into the address struct)
for f in \
Platform/Posix/Source/SolidSyslogPosixAddress.c \
Platform/Windows/Source/SolidSyslogWinsockAddress.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosAddress.c; do
echo "==== $f ===="
rg -n --type=c --type=h 'void (Posix|Winsock|FreeRtos)Address_Initialise' "$f" -S
# print around initialise function
rg -n --type=c 'void (Posix|Winsock|FreeRtos)Address_Initialise' -n "$f" -S | head -n 1
start=$(rg -n 'void (Posix|Winsock|FreeRtos)Address_Initialise' "$f" -S | head -n1 | cut -d: -f1)
# show 40 lines from start (cap to file)
if [ -n "${start}" ]; then
s=$((start-5)); [ "$s" -lt 1 ] && s=1
sed -n "${s},$((start+80))p" "$f"
fi
done
# Resolver signatures: how it takes an address pointer to write into
# Look for the resolver definition header used by platform resolvers
rg -n --type=c --type=h 'typedef.*Resolve|Resolve\(.*struct SolidSyslogAddress\*|struct SolidSyslogAddress\*.*Resolve' Platform Core -S | head -n 80
# Also inspect the platform resolver implementations around where they write into the address.
# (Use small snippets around the Resolve function.)
for f in \
Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c \
Platform/Windows/Source/SolidSyslogWinsockResolver.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c; do
echo "==== $f (Resolve snippet) ===="
rg -n --type=c 'static bool .*_Resolve|Resolve\(' "$f" -S
# print around the first Resolve implementation block
start=$(rg -n --type=c 'static bool .*_Resolve' "$f" -S | head -n1 | cut -d: -f1 || true)
if [ -n "${start}" ]; then
s=$((start-5)); [ "$s" -lt 1 ] && s=1
sed -n "${s},$((start+120))p" "$f"
fi
doneRepository: DavidCozens/solid-syslog
Length of output: 1697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find resolve functions that take a SolidSyslogAddress* and inspect small regions.
rg -n --type=c --type=h 'Resolve\([^)]*struct SolidSyslogAddress\*|Resolve\([^)]*SolidSyslogAddress\*|SolidSyslogAddress\*.*Resolve' Platform -S
# Find where resolvers write into an address object (look for Sockaddr writes)
rg -n --type=c --type=h 'Sockaddr\s*\.|->\s*Sockaddr|Sockaddr\s*=|memcpy\([^,]+,\s*&?[^,]+Sockaddr|freertos_sockaddr|freertos_sockaddr\s*' Platform -S
# Narrow to platform resolver implementations and show their Resolve bodies around first match.
for f in \
Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c \
Platform/Windows/Source/SolidSyslogWinsockResolver.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c; do
echo "==== $f ===="
if [ -f "$f" ]; then
sed -n '1,220p' "$f" | rg -n 'Resolve' || true
rg -n --type=c 'Resolve\(' "$f" -S || true
# print around first Resolve occurrence
line=$(rg -n --type=c 'Resolve\(' "$f" -S | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
s=$((line-20)); [ "$s" -lt 1 ] && s=1
sed -n "${s},$((line+200))p" "$f"
fi
fi
doneRepository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) List resolver implementation files (platform-specific)
# (Use fd if available; otherwise rely on rg to locate known resolver filenames.)
echo "## Resolver files containing 'Resolver' in platform sources:"
rg -n --type=c --type=h 'SolidSyslog.*Resolver|Resolver_Resolve|Resolve\(' Platform -S | cut -d: -f1 | sort -u | head -n 50 || true
# 2) Search for actual address-field writes in all platform sources
echo "## Places writing Sockaddr or freertos_sockaddr:"
rg -n --type=c --type=h --hidden 'Sockaddr\s*[\.\=]|freertos_sockaddr|Sockaddr\s*\.' Platform -S || true
# 3) Specifically inspect resolve implementations by searching for 'Resolve(' and then
# printing a small window around the first few occurrences per file.
for f in \
Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c \
Platform/Windows/Source/SolidSyslogWinsockResolver.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c \
Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolverStatic.c; do
if [ -f "$f" ]; then
echo "==== $f ===="
rg -n --type=c 'Resolve\(' "$f" -S || true
# show first occurrence context if any
line=$(rg -n --type=c 'Resolve\(' "$f" -S | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
s=$((line-30)); [ "$s" -lt 1 ] && s=1
sed -n "${s},$((line+220))p" "$f" | rg -n 'Resolve|Sockaddr|sockaddr_in|freertos_sockaddr|memcpy' -n || true
# show wider raw snippet too (without heavy rg filtering)
sed -n "${s},$((line+220))p" "$f" | head -n 120
fi
fi
done
# 4) Find the address struct field name in each address header
for f in \
Platform/Posix/Interface/SolidSyslogPosixAddress.h \
Platform/Windows/Interface/SolidSyslogWinsockAddress.h \
Platform/FreeRtos/Interface/SolidSyslogFreeRtosAddress.h; do
if [ -f "$f" ]; then
echo "==== $f (field scan) ===="
sed -n '1,200p' "$f" | rg -n 'Sockaddr|sockaddr_in|freertos_sockaddr' || true
fi
doneRepository: DavidCozens/solid-syslog
Length of output: 12826
Fix address pool exhaustion fallback: avoid shared mutable *_Address_Fallback that resolvers overwrite
SolidSyslog{Posix,Winsock,FreeRtos}Address_Create returns a single TU-static fallback instance (PosixAddress_Fallback / WinsockAddress_Fallback / FreeRtosAddress_Fallback) on pool exhaustion, and the code/docs explicitly state multi-overflow callers “share this storage and race”.
The platform resolvers write into the provided struct SolidSyslogAddress* result (e.g., SolidSyslogPosixAddress_AsSockaddrIn(result) with *sin = ... in Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c, the same in Platform/Windows/Source/SolidSyslogWinsockResolver.c, and sockaddr->... writes in Platform/FreeRtos/Source/SolidSyslogFreeRtosStaticResolver.c). With a shared fallback address, concurrent exhaustion can overwrite each other’s address state (data race / incorrect destinations).
Also, there is no SolidSyslogNullAddress symbol/implementation in the repo; addresses use this racing real-address fallback rather than a safe null object like other SolidSyslogNull* fallbacks.
Recommendation: Return a real SolidSyslogNullAddress (or otherwise make the exhaustion fallback immutable/safe for concurrent callers—e.g., per-call storage or explicit synchronization) so resolvers never concurrently write into shared fallback state.
🤖 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 `@CLAUDE.md` around lines 327 - 329, Pool-exhaustion currently returns a shared
mutable fallback (PosixAddress_Fallback / WinsockAddress_Fallback /
FreeRtosAddress_Fallback) that resolvers (Platform/.../SolidSyslog*Resolver.c)
write into, causing races; change the fallback to a safe immutable null object
and return that instead. Implement a SolidSyslogNullAddress (symbol:
SolidSyslogNullAddress and accessors matching SolidSyslogAddress API), update
SolidSyslogPosixAddress_Create, SolidSyslogWinsockAddress_Create,
SolidSyslogFreeRtosAddress_Create to return the null address on pool exhaustion
(remove returning the shared mutable *_Fallback), and update resolvers (e.g.,
SolidSyslogGetAddrInfoResolver.c, SolidSyslogWinsockResolver.c,
SolidSyslogFreeRtosStaticResolver.c) to detect the null address (or use
defensive accessors) so they never write into shared fallback storage. Ensure no
shared mutable *_Fallback is exposed to callers and that the null address is
immutable/safe for concurrent use.
Owner
Author
There was a problem hiding this comment.
Skipping this one — the shared mutable fallback is the agreed design, not an oversight.
Earlier in the S24.07 work I started with exactly your suggestion: a single immutable SolidSyslogNullAddress in Core/Source/. The project owner caught a bug: Core/ can't see <netinet/in.h> / <winsock2.h> / <FreeRTOS_Sockets.h>, so the cross-platform singleton was 1 byte. A platform Resolver writing a 16-byte sockaddr_in into it was silent corruption.
The per-platform mutable fallback fixes that — it's sized as a real SolidSyslog<Plat>Address, so a Resolver overwrite is bounded. The race you're flagging is real but is the integrator's signal to bump SOLIDSYSLOG_ADDRESS_POOL_SIZE: the SOLIDSYSLOG_ERROR_MSG_*ADDRESS_POOL_EXHAUSTED error handler fires on every exhaustion. The DEVLOG entry for S24.07 documents the trade-off explicitly.
An immutable null sized for the worst-case sockaddr would re-introduce the same Core/-side coupling problem we just escaped.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — thank you for the detailed explanation. The Core/-side platform-header isolation constraint is the key piece I was missing; a cross-platform singleton in Core/Source/ necessarily can't include <netinet/in.h> / <winsock2.h> / <FreeRTOS_Sockets.h>, so it would be undersized and a resolver write into it would be silent out-of-bounds corruption. The per-platform mutable fallback, correctly sized as the real SolidSyslog<Plat>Address, is the right solution given that constraint. The documented exhaustion-error + pool-size tunable provides the integrator signal to prevent repeated hits. I'll record this for future reviews.
(´• ω •`) ♡
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 422
File: CLAUDE.md:327-329
Timestamp: 2026-05-22T12:14:57.813Z
Learning: In the solid-syslog project (DavidCozens/solid-syslog), the pool-exhaustion fallback for SolidSyslogPosixAddress, SolidSyslogWinsockAddress, and SolidSyslogFreeRtosAddress is intentionally a per-platform TU-private mutable singleton (e.g. PosixAddress_Fallback, WinsockAddress_Fallback, FreeRtosAddress_Fallback), NOT a shared immutable SolidSyslogNullAddress. The reason: Core/ cannot include platform socket headers (<netinet/in.h>, <winsock2.h>, <FreeRTOS_Sockets.h>), so a Core-level cross-platform singleton would be only 1 byte, causing silent corruption when platform resolvers write a 16-byte sockaddr_in into it. The per-platform fallback is correctly sized as the real SolidSyslog<Plat>Address, bounding resolver overwrites. The documented trade-off (race on multi-overflow) is the integrator's signal to bump SOLIDSYSLOG_ADDRESS_POOL_SIZE; the SOLIDSYSLOG_ERROR_MSG_*ADDRESS_POOL_EXHAUSTED error fires on every exhaustion. This design is documented in the S24.07 DEVLOG entry.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| self->Base.Disconnect = StreamSender_Disconnect; | ||
| self->Config.Resolver = config->Resolver; | ||
| self->Config.Stream = config->Stream; | ||
| self->Config.Address = config->Address; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
There was a problem hiding this comment.
Valid finding, deferred to its own story.
StreamSender_Create has never validated NULL Resolver / NULL Stream / NULL Endpoint either — it's been permissive since well before S24.07. Adding NULL-Address validation alone in this PR would leave the bad-setup contract inconsistent (only one of four required fields covered). Adding the full contract would be scope creep on a story that's already large.
Raised as #424 — S24.09: StreamSender bad-setup contract — reject NULL Resolver / Stream / Address. Linked as a sub-issue of E24 (#254) and added to the project board with Status=Todo. The story covers all four NULL paths plus the matching tests, mirroring the existing SolidSyslogUdpSenderBadSetup shape.
The S24.07 DEVLOG entry records the deferral.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@DavidCozens, the rationale is solid — adding a single NULL-Address guard while leaving Resolver, Stream, and Endpoint unchecked would create an inconsistent contract and is rightly out of scope for S24.07. Tracking it holistically in #424 alongside the full bad-setup contract and mirrored tests is the right approach. I'll resolve this comment now.
🏁 Script executed:
#!/bin/bash
# Resolve the review comment via GitHub CLI
gh pr view 422 --repo DavidCozens/solid-syslog --json reviews,comments 2>/dev/null | head -n 5 || true
# List review comments on the PR to find the one to resolve
gh api repos/DavidCozens/solid-syslog/pulls/422/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}' 2>/dev/null | head -n 40 || true
Length of output: 91768
🏁 Script executed:
#!/bin/bash
# Resolve the review comment using GitHub GraphQL minimizeComment mutation
# The comment node ID from the REST API id is 3287356614 -> need to get the node ID
# Get the node ID for comment 3287356614
gh api repos/DavidCozens/solid-syslog/pulls/422/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, node_id: .node_id, body: .body[0:60]}' 2>/dev/null
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Minimize/resolve the review comment PRRC_kwDORzQvQs7D8RTG
gh api graphql -f query='
mutation {
minimizeComment(input: {subjectId: "PRRC_kwDORzQvQs7D8RTG", classifier: RESOLVED}) {
minimizedComment {
isMinimized
minimizedReason
}
}
}' 2>/dev/null
Length of output: 99
@DavidCozens, the rationale is solid — adding only NULL-Address validation would leave an inconsistent contract across the four required fields, and folding the full bad-setup story into S24.07 would be out of scope. Tracking it comprehensively in #424 alongside the matching SolidSyslogUdpSenderBadSetup-shaped tests is the right call. I've resolved this comment.
🧬 Code Graph Analysis Results
- File: Core/Source/SolidSyslogNullSender.c (lines 13-16)
struct SolidSyslogSender* SolidSyslogNullSender_Get(void)
{
return &instance;
}
- File: Core/Source/SolidSyslogResolver.c (lines 10-19)
bool SolidSyslogResolver_Resolve(
struct SolidSyslogResolver* resolver,
enum SolidSyslogTransport transport,
const char* host,
uint16_t port,
struct SolidSyslogAddress* result
)
{
return resolver->Resolve(resolver, transport, host, port, result);
}
- File: Core/Source/SolidSyslogStream.c (lines 9-12)
bool SolidSyslogStream_Open(struct SolidSyslogStream* stream, const struct SolidSyslogAddress* addr)
{
return stream->Open(stream, addr);
}
- File: Core/Source/SolidSyslogStream.c (lines 14-17)
bool SolidSyslogStream_Send(struct SolidSyslogStream* stream, const void* buffer, size_t size)
{
return stream->Send(stream, buffer, size);
}
- File: Core/Source/SolidSyslogStream.c (lines 24-27)
void SolidSyslogStream_Close(struct SolidSyslogStream* stream)
{
stream->Close(stream);
}
- File: Core/Source/SolidSyslogStream.c (additional context for Open/Close usage)
bool SolidSyslogStream_Open(struct SolidSyslogStream* stream, const struct SolidSyslogAddress* addr)
{
return stream->Open(stream, addr);
}
- File: Core/Source/SolidSyslogFormatter.c (lines 91-98)
struct SolidSyslogFormatter* SolidSyslogFormatter_Create(SolidSyslogFormatterStorage* storage, size_t bufferSize)
{
struct SolidSyslogFormatter* formatter = (struct SolidSyslogFormatter*) storage;
formatter->Size = bufferSize;
formatter->Position = 0;
Formatter_NullTerminate(formatter);
return formatter;
}
- File: Core/Source/SolidSyslogFormatter.c (lines 489-493)
const char* SolidSyslogFormatter_AsFormattedBuffer(struct SolidSyslogFormatter* formatter)
{
Formatter_TrimTruncatedMultiByteTail(formatter);
return formatter->Buffer;
}
- File: Core/Source/SolidSyslogFormatter.c (lines 526-529)
size_t SolidSyslogFormatter_Length(const struct SolidSyslogFormatter* formatter)
{
return formatter->Position;
}
- File: Core/Source/SolidSyslogFormatter.c (lines 425-442)
void SolidSyslogFormatter_Uint32(struct SolidSyslogFormatter* formatter, uint32_t value)
{
size_t digits = Formatter_CountDigits(value);
uint32_t divisor = 1;
for (size_t i = 1; i < digits; i++)
{
divisor *= 10U;
}
for (size_t i = 0; i < digits; i++)
{
Formatter_WriteChar(formatter, Formatter_DigitToChar(value / divisor));
value %= divisor;
divisor /= 10U;
}
Formatter_NullTerminate(formatter);
}
- File: Core/Source/SolidSyslogFormatter.c (lines 108-116)
void SolidSyslogFormatter_AsciiCharacter(struct SolidSyslogFormatter* formatter, char value)
{
if (!Formatter_IsAsciiCharacter(value))
{
value = NON_PRINTABLE_SUBSTITUTE;
}
Formatter_WriteChar(formatter, value);
Formatter_NullTerminate(formatter);
}
- File: Core/Source/SolidSyslogFormatter.c (lines 161-181)
void SolidSyslogFormatter_BoundedString(struct SolidSyslogFormatter* formatter, const char* source, size_t maxLength)
{
size_t len = 0;
while ((len < maxLength) && (source[len] != '\0'))
{
size_t codepointLength = Formatter_Utf8CodepointLength(&source[len]);
if (Formatter_CodepointFits(codepointLength, maxLength - len))
{
Formatter_WriteBytes(formatter, &source[len], codepointLength);
len += codepointLength;
}
else
{
Formatter_WriteBytes(formatter, REPLACEMENT_CHARACTER, sizeof(REPLACEMENT_CHARACTER));
len += 1U;
}
}
Formatter_NullTerminate(formatter);
}
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 422
File: Core/Source/SolidSyslogStreamSender.c:55-55
Timestamp: 2026-05-22T12:17:01.351Z
Learning: In DavidCozens/solid-syslog, `SolidSyslogStreamSender_Create` has never validated NULL config fields (Resolver, Stream, Address, Endpoint). Full create-time bad-setup contract validation (reject NULL Resolver/Stream/Address/Endpoint, with matching tests mirroring `SolidSyslogUdpSenderBadSetup`) is tracked in issue `#424` (S24.09), linked as a sub-issue of E24 (`#254`). Do not flag the missing NULL checks in StreamSender_Create as an issue in future PRs until `#424` is addressed.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 422
File: Core/Source/SolidSyslogStreamSender.c:55-55
Timestamp: 2026-05-22T12:17:01.351Z
Learning: In DavidCozens/solid-syslog, `SolidSyslogStreamSender_Create` has never validated NULL config fields (Resolver, Stream, Address, Endpoint). Full create-time bad-setup contract validation (reject NULL Resolver/Stream/Address/Endpoint, with matching tests mirroring `SolidSyslogUdpSenderBadSetup`) is tracked in issue `#424` (S24.09), linked as a sub-issue of E24 (`#254`). Do not flag missing NULL checks in `StreamSender_Create` as an issue in future PRs until `#424` is addressed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:363-363
Timestamp: 2026-05-07T08:22:59.856Z
Learning: In this solid-syslog codebase, follow the documented contract: do not add NULL/defensive checks inside library/internal code paths (e.g., `_Create` functions) when the NULL case cannot occur under normal internal usage and is covered by framework or internal wiring guarantees. Only add validation at true system boundaries such as user input and external APIs. Where setup-time wiring contracts between the integrator and the library require specific function-pointer fields (e.g., `SolidSyslogTlsStreamConfig.sleep`), assume those required fields are present and intentionally omit NULL guards in internal creation functions.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 291
File: Example/FreeRtos/HelloWorld/CmsdkUart.c:17-42
Timestamp: 2026-05-08T17:19:38.382Z
Learning: In this codebase (DavidCozens/solid-syslog), internal driver/config/access structs may intentionally store raw pointers without NULL checks, `isInitialized` flags, or defensive struct copies. Treat this as an explicit project policy: do not flag store-by-pointer usage or missing NULL/initialization guards for these internal config/access structs when the pointer is provided by production callers and is guaranteed to point to `static const` objects with program lifetime (per CLAUDE.md). Only flag NULL/initialization issues when the pointer originates from potentially invalid/external/untrusted sources or when the code does not establish lifetime/guarantees.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/Common/ExampleInteractive.h:14-17
Timestamp: 2026-05-09T15:56:13.853Z
Learning: During code review, ensure solid-syslog Tier 1/2 library callbacks in Core/ and Platform/ follow the CLAUDE.md “Callback Conventions”: the callback API must take a `void*` context parameter and the implementation must use the paired configuration field associated with that callback context. Do not apply this requirement to Tier 3 example handlers (e.g., Example/Common/, Example/FreeRtos/, such as `ExampleInteractiveSwitchHandler`/`ExampleInteractiveSetHandler`), which are allowed to use file-static globals per the documented example-tier pattern. If a multi-instance use-case arises, migrate Switch and Set together in a single cohesive change rather than piecemeal updates.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 372
File: Platform/FreeRtos/Source/SolidSyslogFreeRtosDatagram.c:0-0
Timestamp: 2026-05-15T13:09:11.811Z
Learning: When reviewing PRs that bulk-rename identifiers (especially “static function rename” changes) in this repo, check for unintended renames inside FreeRTOS macro usages. A too-broad regex can incorrectly prefix macro tokens used in `static const` initializers (e.g., `pdMS_TO_TICKS` from FreeRTOS). Look for signs like a renamed macro name being incorrectly prefixed (e.g., `FreeRtosDatagram_pdMS_TO_TICKS`), and if present, treat it as evidence the identifier-extraction regex was overly broad and requires correction/revert of the affected rename changes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 400
File: Core/Source/SolidSyslogOriginSdStatic.c:11-11
Timestamp: 2026-05-18T17:07:19.499Z
Learning: In this repo (DavidCozens/solid-syslog), an IWYU CI gate enforces strict direct-header inclusion: for every symbol (types, enums, functions, etc.) that is directly referenced in a translation unit’s `.c` or `.h` file, include the specific header that declares that symbol. Do not rely on transitive includes from other headers.
Example: if a `.c`/`.h` file uses `SolidSyslogSeverity` enums, it must directly `#include` `SolidSyslogPrival.h` even if another included header (e.g., `SolidSyslogError.h`) pulls it in transitively.
When reviewing, do not flag or recommend removing includes solely because they are “transitive” in this codebase—removing them may still compile locally but can fail the IWYU CI check.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 402
File: Core/Source/SolidSyslogFileBlockDeviceStatic.c:43-52
Timestamp: 2026-05-19T06:57:12.659Z
Learning: In this repo’s SolidSyslog code, some pool-backed Create functions fall back to a shared null-object handle on pool exhaustion (e.g., SolidSyslogFileBlockDevice_Create / SolidSyslogStreamSender_Create). When that null-object handle reaches the corresponding Destroy function, that Destroy is intentionally expected to emit the UNKNOWN_DESTROY warning for that handle as a deliberate dual-signal design (POOL_EXHAUSTED at Create time plus UNKNOWN_DESTROY at Destroy time to surface pool misconfiguration). During code review, do not suggest suppressing, removing, or downgrading the Destroy-time UNKNOWN_DESTROY warning specifically for the null-object fallback handle.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 416
File: DEVLOG.md:155-155
Timestamp: 2026-05-20T17:56:08.543Z
Learning: When a source file needs the public facility/severity enums used by `SolidSyslog_Error` (or other public APIs), include the public header `Core/Interface/SolidSyslogPrival.h` rather than `Core/Source/SolidSyslogPrivate.h`. Reserve `Core/Source/SolidSyslogPrivate.h` (TU-internal structs like `SolidSyslog`) for the translation unit that defines/uses them (e.g., `SolidSyslogStatic.c`).
…ast + cppcheck Addresses four CI failures and one CodeRabbit miss on PR #422: - build-freertos-target: Bdd/Targets/FreeRtos/CMakeLists.txt still listed the deleted Platform/FreeRtos/Source/SolidSyslogAddress.c. Swapped to the new SolidSyslogFreeRtosAddress{.c,Static.c} pair; updated the stale Address.h / AddressInternal.h header comments. - build-windows-msvc + integration-windows-openssl: five test / integration files hard-coded `#include "SolidSyslogPosixAddress.h"`, which doesn't exist on Windows. Introduces Tests/AddressFake.{h,c} — a tiny cross-platform opaque-singleton helper for tests that treat SolidSyslogAddress* purely as a pass-through token (StreamFakeTest, TlsStreamTest, MbedTlsStreamTest, both TLS integration tests). Wired into each test executable's CMakeLists (SolidSyslogTests, MbedTlsStreamTest, MbedTlsIntegrationTests, OpenSslIntegrationTests). - analyze-cppcheck: two unreadVariable warnings on the SolidSyslogGetAddrInfoResolver fixture I refactored. Added the standard cppcheck-suppress comments matching the convention used elsewhere — cppcheck does not model CppUTest TEST_GROUP lifecycle. - CodeRabbit comment (outside diff range): one missed migration in Tests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cpp — replaced reinterpret_cast<const struct freertos_sockaddr*>(addr) with SolidSyslogFreeRtosAddress_AsConstFreertosSockaddr(addr) to match the equivalent fix in SolidSyslogFreeRtosDatagramTest.cpp. Verified locally in the cpputest-freertos image: SolidSyslogTests 1290/1292; all six FreeRTOS host tests green; MbedTlsStreamTest 42/42; OpenSslIntegrationTests 9/9; MbedTlsIntegrationTests 7/7; cppcheck preset clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 22, 2026
Owner
Author
|
Responses to the two CodeRabbit comments that weren't posted inline: Outside-diff range —
|
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1296 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
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.
Purpose
Closes #418 (E24's last open story). Retires the
SolidSyslogAddressStoragecaller-supplied-storage pattern — the lastholdout from E11's pool migration — by folding
SolidSyslogAddressonto the same pool/handle pattern that every other Created class uses.
The old shape forced every consumer to carry 128 B per Address slot
(sized for
sockaddr_storagefor cross-platform parity) and survivedD.002(b) only because Address had no Create/Destroy lifecycle.
Post-merge each platform's Address pool slot is exactly one platform
sockaddr_in/freertos_sockaddr(~16 B), the D.002(b) deviationretires, and the design is internally consistent.
Change Description
SolidSyslogPosixAddress,SolidSyslogWinsockAddress,SolidSyslogFreeRtosAddress— eachwraps the platform sockaddr in the standard 3-TU split
(
.c/Private.h/Static.c) used by every other E11 poolclass. Pool size shared via
SOLIDSYSLOG_ADDRESS_POOL_SIZE(default 3 — covers the canonical UDP + plain-TCP + TLS-stream
BDD wiring, same trade-off as
SOLIDSYSLOG_POSIX_TCP_STREAM_POOL_SIZE).*AddressStatic.cholds a TU-private
<Plat>Address_Fallbacksingleton sized as areal
SolidSyslog<Plat>Address, so a Resolver overwrite onexhaustion is bounded (no UB). The first attempt at this used a
cross-platform
SolidSyslogNullAddresssingleton inCore/Source/;reverted because
Corecan't see the platform sockaddr headersand would have been UB-prone. See DEVLOG.
UdpSenderConfig.Address/StreamSenderConfig.Address: newrequired field.
UdpSenderdrops its inline 128-byteAddrStoragefield;
StreamSender_Connectdrops its per-Connect stackallocation. UdpSender adds NULL-Address validation to its existing
bad-setup contract; StreamSender keeps its existing permissive
posture.
Posix/Winsock/FreeRtos Resolver/Datagram/Stream files swap from
the old shared
SolidSyslogAddress_AsSockaddrInetc. onto theper-platform
SolidSyslog<Plat>Address_AsSockaddrInin each new*AddressPrivate.h.SolidSyslogAddressStorageontothe per-platform Address pool — POSIX / Winsock / FreeRTOS
datagram + stream + resolver tests, both TLS unit tests, both TLS
integration tests,
StreamFakeTest.Sender (UDP + plain-TCP + TLS), exactly fitting pool default 3.
SolidSyslogAddressStorage,SOLIDSYSLOG_ADDRESS_SIZE,SolidSyslogAddress_FromStorage, thethree identical
Platform/*/Source/SolidSyslogAddress.cfactoryfiles, the three
SolidSyslogAddressInternal.hheaders, andTests/SolidSyslogAddressTest.cpp.docs/misra-deviations.mdD.002 narrowed: the (b) Address-as-value-type paragraph retires;
Address casts fold into D.002(a)'s standard opaque-handle downcast
scope. CLAUDE.md public-header table updated.
Test Evidence
Behavioural changes were TDD'd: each platform's
_Create/_Destroy/ accessor / NULL-Address-rejection driven via red/greenon the existing pool-class test template. The pool plumbing
(lock-count, error reporting, stale-destroy warning) was
duplicate-and-rename from
SolidSyslogFreeRtosStaticResolverPoolper agreement, with the standard 10 tests per class.
Local sweep (cpputest-freertos host image):
SolidSyslogTests— 1290/1292 (2 ignored)BddTargetTests— 48/48SolidSyslogFreeRtosAddressTest— 11/11 (new)SolidSyslogFreeRtosDatagramTest— 30/30SolidSyslogFreeRtosMutexTest— 13/13SolidSyslogFreeRtosStaticResolverTest— 18/18SolidSyslogFreeRtosSysUpTimeTest— 4/4SolidSyslogFreeRtosTcpStreamTest— 46/46SolidSyslogMbedTlsStreamTest— 42/42OpenSslIntegrationTests— 9/9MbedTlsIntegrationTests— 7/7readability-make-member-function-constfix)Windows + BDD-Linux/Windows/FreeRTOS QEMU + integration-Windows
verification deferred to CI.
Areas Affected
Core/Interface/,Core/Source/):SolidSyslogAddress.h,SolidSyslog{Udp,Stream}Sender.h,SolidSyslog{Udp,Stream}Sender{.c,Private.h,Static.c},SolidSyslogTunablesDefaults.h,SolidSyslogErrorMessages.h.Platform/{Posix,Windows,FreeRtos}/): new Addresspool class trio per platform; Resolver / Datagram / Stream
consumers swap accessor names.
Bdd/Targets/): Linux / Windows / FreeRTOS main + thethree Common TLS-sender glue files wire an Address handle per Sender.
Tests/,docs/): 13 test fixtures migrated;D.002 paragraph in
docs/misra-deviations.mdretired; CLAUDE.mdpublic-header table updated.
Breaking change:
SolidSyslogAddressStorageandSolidSyslogAddress_FromStorageare removed. Integrators mustallocate destination addresses through the platform pool
(
SolidSyslog{Posix,Winsock,FreeRtos}Address_Create) and wire thereturned handle into
UdpSenderConfig.Address/StreamSenderConfig.Address.Closes #418
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
API Changes
Configuration
Error Handling