feat: S11.05 part A — public storage-cast classes onto PoolAllocator + shared null objects#402
Merged
Merged
Conversation
Per the E11 canonical 3-TU split. Internal struct moves to
SolidSyslogStreamSenderPrivate.h; vtable + Initialise/Cleanup stay
in SolidSyslogStreamSender.c; pool, public Create/Destroy,
IndexFromHandle, CleanupAtIndex live in SolidSyslogStreamSenderStatic.c.
Pool exhaustion resolves to the shared SolidSyslogNullSender that
shipped with S11.04 — no class-private fallback needed.
Public API: drops the SolidSyslogStreamSenderStorage typedef + the
SOLIDSYSLOG_STREAM_SENDER_SIZE enum from the header; _Create loses
the storage parameter. _Destroy keeps the (struct SolidSyslogSender*)
shape established by S11.04.
Pool size tunable: SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE, default 1.
Error messages: STREAMSENDER_POOL_EXHAUSTED (ERROR severity) and
STREAMSENDER_UNKNOWN_DESTROY (WARNING severity).
Pool test: new TEST_GROUP(SolidSyslogStreamSenderPool) with
FillingPoolThenOverflowReturnsDistinctFallback. Generic per-probe
lock and stale-handle behaviour is covered by
SolidSyslogPoolAllocatorTest.cpp.
NoEndpointConfiguredConnectsToPortZero rewired — the original
implicitly relied on singleton semantics (second Create silently
overwrote the slot). Pool semantics require explicit Destroy then
reassign so the second Create draws a fresh slot.
CreateReturnsHandleInsideCallerSuppliedStorage dropped — it verified
the storage-cast invariant that no longer exists.
Caller updates: Bdd/Targets/{Linux,Windows,FreeRtos}/main.c drop
their `static SolidSyslogStreamSenderStorage` lines; the two
BddTargetTlsSender_OpenSsl_*Tcp.c variants likewise.
MISRA suppressions: deletes the old SelfFromStorage 11.3 entry and
the header 5.7 entry (both refer to constructs that no longer
exist); updates SelfFromBase 11.3 line to :150 and adds the
anonymous-enum 2.4/5.7 entries at the new line. Branch vs main
cppcheck-misra diff: zero new findings, one evaporated (8.9 on
the deleted DEFAULT_INSTANCE / DESTROYED_INSTANCE).
Validated at SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE=3 via
SOLIDSYSLOG_USER_TUNABLES_FILE override — full suite green
(1155 tests). 100% line coverage on both new TUs.
CLAUDE.md audience-table row updated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pool slots previously NULL'd their abstract-base vtable in _Cleanup, so calling Send/Disconnect/Write/Read/Format through a stale handle after Destroy was a NULL-fn-pointer dereference. The original pre-E11 pattern (`*self = DESTROYED_INSTANCE;`) had the same hazard at the vtable site. Replace the NULL-out with a copy of the relevant null-object vtable into the slot's abstract base. Use-after-destroy now routes through the GoF null implementation — Send returns true (drop-on-floor), Disconnect/Write/Format no-op, Read returns false with bytesRead=0. Only the abstract base (the public-facing first member of every derived slot struct) needs the swap. Derived fields (Config, Connected, LastEndpointVersion, Sender, Ring, ...) are private to each TU; no public API can reach them, and the next _Initialise overwrites the whole struct on slot reuse. Per-class shape: - StreamSender/UdpSender: Disconnect first (still using the live Config.Stream/Datagram), then `*base = *SolidSyslogNullSender_Get()`. - SwitchingSender: bare swap — does not own inner-sender connections. - MetaSd/TimeQualitySd/OriginSd: bare swap to NullSd. - PassthroughBuffer/CircularBuffer: no shared GoF NullBuffer exists, so the existing class-private Fallback (lived as `static struct SolidSyslogBuffer Fallback` in *Static.c) is exposed as `<Class>_Fallback` via the Private.h header (drops `static`, gains the Class prefix). _Cleanup copies it into the slot's Base, and _Create's pool-exhausted fallback path uses the same symbol. Tests: one UseAfterDestroyIsCrashSafeViaXxxVtable test per class exercises Send/Format/Write/Read after Destroy and asserts the null-object semantics. For groups whose teardown auto-destroys the slot, the test re-creates via the same Create at the end so teardown still releases a live slot. MISRA delta (cppcheck-misra vs main): zero new findings, three evaporated (rule 8.9 on StreamSender's DEFAULT/DESTROYED_INSTANCE deletion and on PassthroughBuffer's / CircularBuffer's Fallback losing its `static` qualifier). Total findings 94 vs main's 98. Coverage 99.6% — unchanged from commit 1 (the new tests exercise the null-object paths that were already covered by existing pool-exhaustion tests; the new Cleanup bodies are shorter). All gates green: debug (1163 tests), clang-debug (1163), sanitize (1163), coverage, tidy, cppcheck, cppcheck-misra, clang-format, IWYU. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ckDevice + NullBuffer
Three things land together because they are the same architectural choice:
1. FileBlockDevice migration to the canonical E11 3-TU split.
- SolidSyslogFileBlockDevice.c: vtable + Initialise/Cleanup.
- SolidSyslogFileBlockDevicePrivate.h: struct + Initialise/Cleanup decls.
- SolidSyslogFileBlockDeviceStatic.c: pool + Create/Destroy +
IndexFromHandle + CleanupAtIndex.
- Public header: SolidSyslogFileBlockDeviceStorage typedef and
SOLIDSYSLOG_FILE_BLOCK_DEVICE_STORAGE_SIZE enum deleted; _Create loses
the storage parameter; _Destroy keeps the (struct *BlockDevice*) shape.
- Pool size tunable: SOLIDSYSLOG_FILE_BLOCK_DEVICE_POOL_SIZE, default 1.
- Error messages: FILEBLOCKDEVICE_POOL_EXHAUSTED (ERROR) and
FILEBLOCKDEVICE_UNKNOWN_DESTROY (WARNING).
- Pool test: TEST_GROUP(SolidSyslogFileBlockDevicePool) with
FillingPoolThenOverflowReturnsDistinctFallback.
- Use-after-destroy test exercises the post-Cleanup vtable.
2. Two new shared GoF nulls — SolidSyslogNullBlockDevice and
SolidSyslogNullBuffer. Both are public Get-only objects mirroring the
existing NullSender / NullSd / NullStore family.
- NullBlockDevice: every method returns false / 0 (disk doesn't exist).
- NullBuffer: Write swallows; Read returns false with bytesRead=0.
3. PassthroughBuffer / CircularBuffer retrofit. Their class-private
`Fallback` static (defined in *Static.c, exposed by *Private.h in
commit 1b) is deleted; their _Cleanup now installs NullBuffer's vtable,
and their pool-exhausted _Create returns NullBuffer_Get() instead.
Same retrofit for FileBlockDevice landing fresh — no class-private
Fallback ever exists; it uses NullBlockDevice from day one.
Why these three land together:
- S11.04 had explicitly deferred the shared NullBuffer because
"PassthroughBuffer is itself the closest thing to a 'just forward'
Buffer". Commit 1b changed the calculus: every Cleanup now needs a
null-object vtable, so a duplicated class-private Fallback per class
becomes copy-paste pressure. A shared null absorbs that cleanly.
- FileBlockDevice arriving fresh would have introduced the same
class-private Fallback pattern — and brought one MORE bare-prefixed
set of Fallback_* statics colliding with the existing two on MISRA
5.9 (advisory: internal-linkage uniqueness across TUs). Reaching for
a shared NullBlockDevice avoids the new collision AND lets us
retrofit the existing two buffer classes in the same touch.
MISRA cppcheck-misra delta vs main: 92 findings (was 98). Five
evaporated (CircularBufferStatic 5.9 + 8.9, PassthroughBufferStatic 5.9
+ 8.9, StreamSender 8.9 from commit 1's DEFAULT/DESTROYED_INSTANCE
deletion). Two new (NullBlockDevice 8.9 + NullBuffer 8.9 — the static
`instance` variable, matching the existing pattern for NullSender /
NullSd / NullStore on main).
Caller updates: drop SolidSyslogFileBlockDeviceStorage locals from
Tests/SolidSyslog{BlockStore,BlockStoreDrainOrdering,BlockStorePosix,
FileBlockDevice}Test.cpp and Bdd/Targets/{Linux,Windows,FreeRtos}/*.
The _Create call sites drop the storage argument.
Coverage 99.6% (2515 of 2525 lines, no uncovered lines in the four new
TUs). Validated at SOLIDSYSLOG_FILE_BLOCK_DEVICE_POOL_SIZE=3 via
SOLIDSYSLOG_USER_TUNABLES_FILE override — full suite green.
All gates green: debug (1178 tests), clang-debug, sanitize, coverage,
tidy, cppcheck, cppcheck-misra, clang-format, IWYU.
CLAUDE.md updates: two new audience-table rows for the shared nulls;
FileBlockDevice / PassthroughBuffer / CircularBuffer rows updated to
reference the new shared fallback target.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the three commits (StreamSender + crash-safety retrofit + FileBlockDevice migration + shared NullBlockDevice / NullBuffer) and flags what's deferred to part B (the BlockStore composition migration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 (8)
📝 WalkthroughWalkthroughThis PR migrates ChangesPool Allocator Migration with Null-Object Crash-Safety Retrofits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1182 passed, 🙈 2 skipped) Created 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: 4
🤖 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`:
- Line 343: Pick one English variant for “synchronization/synchronised” and
apply it consistently across the document; search for all occurrences (e.g., the
phrase in the SolidSyslogCircularBuffer.h description that currently reads
“mutex-injected synchronization” and any instances of “synchronised” elsewhere)
and replace them to the chosen spelling, keeping other terms and symbols
unchanged (references like SOLIDSYSLOG_CIRCULAR_BUFFER_HEADER_BYTES,
SOLIDSYSLOG_CIRCULAR_BUFFER_RING_BYTES, SolidSyslogCircularBuffer_Create,
_Destroy, SolidSyslogNullBuffer should remain exact).
In `@Core/Source/SolidSyslogFileBlockDeviceStatic.c`:
- Around line 43-52: The destroy routine SolidSyslogFileBlockDevice_Destroy
currently warns when freeing a handle that is actually the overflow fallback
returned by SolidSyslogNullBlockDevice_Get(); update the function to detect that
fallback (compare the incoming base to SolidSyslogNullBlockDevice_Get() or
otherwise detect the null/fallback handle) and return without logging an error,
otherwise proceed with the existing index lookup and call to
SolidSyslogPoolAllocator_FreeIfInUse (using FileBlockDevice_IndexFromHandle,
SolidSyslogPoolAllocator_IndexIsValid, SolidSyslogPoolAllocator_FreeIfInUse and
FileBlockDevice_CleanupAtIndex) so valid pool frees still occur and the
SOLIDSYSLOG_ERROR_MSG_FILEBLOCKDEVICE_UNKNOWN_DESTROY warning is only emitted
for truly unknown/unmanaged handles.
In `@Core/Source/SolidSyslogStreamSenderStatic.c`:
- Around line 42-51: SolidSyslogStreamSender_Destroy currently logs a warning
when called with the overflow fallback/null sender returned by Create; update
the function to detect the null/fallback sender (compare the passed base pointer
to SolidSyslogNullSender_Get() or use an equivalent "is null sender" check) and
silently return without logging; keep the existing allocator/index checks
(StreamSender_IndexFromHandle and
SolidSyslogPoolAllocator_IndexIsValid/FreeIfInUse) for normal senders, but skip
the warning path when base is the null/fallback sender.
In `@DEVLOG.md`:
- Around line 33-34: Replace the typo "user-after-destroy" with the correct
technical term "use-after-destroy" in the DEVLOG entry so the sentence reads
"...the use-after-destroy crash-safety gap..." (search for the exact string
"user-after-destroy" in the document to locate and update it).
🪄 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: f764b2fa-d2f9-4e8d-83f9-bf95b087e7f4
📒 Files selected for processing (47)
Bdd/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/SolidSyslogFileBlockDevice.hCore/Interface/SolidSyslogNullBlockDevice.hCore/Interface/SolidSyslogNullBuffer.hCore/Interface/SolidSyslogStreamSender.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/CMakeLists.txtCore/Source/SolidSyslogCircularBuffer.cCore/Source/SolidSyslogCircularBufferStatic.cCore/Source/SolidSyslogErrorMessages.hCore/Source/SolidSyslogFileBlockDevice.cCore/Source/SolidSyslogFileBlockDevicePrivate.hCore/Source/SolidSyslogFileBlockDeviceStatic.cCore/Source/SolidSyslogMetaSd.cCore/Source/SolidSyslogNullBlockDevice.cCore/Source/SolidSyslogNullBuffer.cCore/Source/SolidSyslogOriginSd.cCore/Source/SolidSyslogPassthroughBuffer.cCore/Source/SolidSyslogPassthroughBufferStatic.cCore/Source/SolidSyslogStreamSender.cCore/Source/SolidSyslogStreamSenderPrivate.hCore/Source/SolidSyslogStreamSenderStatic.cCore/Source/SolidSyslogSwitchingSender.cCore/Source/SolidSyslogTimeQualitySd.cCore/Source/SolidSyslogUdpSender.cDEVLOG.mdTests/CMakeLists.txtTests/SolidSyslogBlockStoreDrainOrderingTest.cppTests/SolidSyslogBlockStorePosixTest.cppTests/SolidSyslogBlockStoreTest.cppTests/SolidSyslogCircularBufferTest.cppTests/SolidSyslogFileBlockDeviceTest.cppTests/SolidSyslogMetaSdTest.cppTests/SolidSyslogNullBlockDeviceTest.cppTests/SolidSyslogNullBufferTest.cppTests/SolidSyslogOriginSdTest.cppTests/SolidSyslogPassthroughBufferTest.cppTests/SolidSyslogStreamSenderTest.cppTests/SolidSyslogSwitchingSenderTest.cppTests/SolidSyslogTimeQualitySdTest.cppTests/SolidSyslogUdpSenderTest.cppmisra_suppressions.txt
💤 Files with no reviewable changes (1)
- Core/Interface/SolidSyslogFileBlockDevice.h
Comment on lines
+43
to
+52
| void SolidSyslogFileBlockDevice_Destroy(struct SolidSyslogBlockDevice* base) | ||
| { | ||
| size_t index = FileBlockDevice_IndexFromHandle(base); | ||
| bool released = | ||
| SolidSyslogPoolAllocator_IndexIsValid(&FileBlockDevice_Allocator, index) && | ||
| SolidSyslogPoolAllocator_FreeIfInUse(&FileBlockDevice_Allocator, index, FileBlockDevice_CleanupAtIndex, NULL); | ||
| if (!released) | ||
| { | ||
| SolidSyslog_Error(SOLIDSYSLOG_SEVERITY_WARNING, SOLIDSYSLOG_ERROR_MSG_FILEBLOCKDEVICE_UNKNOWN_DESTROY); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid warning on destroy of the overflow fallback block device.
If Create exhausts the pool (Line 29/Line 38), it returns SolidSyslogNullBlockDevice_Get(). Destroying that fallback currently always emits SOLIDSYSLOG_ERROR_MSG_FILEBLOCKDEVICE_UNKNOWN_DESTROY (Line 51), although the lifecycle is valid.
Proposed fix
void SolidSyslogFileBlockDevice_Destroy(struct SolidSyslogBlockDevice* base)
{
+ if (base == SolidSyslogNullBlockDevice_Get())
+ {
+ return;
+ }
+
size_t index = FileBlockDevice_IndexFromHandle(base);
bool released =
SolidSyslogPoolAllocator_IndexIsValid(&FileBlockDevice_Allocator, index) &&
SolidSyslogPoolAllocator_FreeIfInUse(&FileBlockDevice_Allocator, index, FileBlockDevice_CleanupAtIndex, NULL);🤖 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 `@Core/Source/SolidSyslogFileBlockDeviceStatic.c` around lines 43 - 52, The
destroy routine SolidSyslogFileBlockDevice_Destroy currently warns when freeing
a handle that is actually the overflow fallback returned by
SolidSyslogNullBlockDevice_Get(); update the function to detect that fallback
(compare the incoming base to SolidSyslogNullBlockDevice_Get() or otherwise
detect the null/fallback handle) and return without logging an error, otherwise
proceed with the existing index lookup and call to
SolidSyslogPoolAllocator_FreeIfInUse (using FileBlockDevice_IndexFromHandle,
SolidSyslogPoolAllocator_IndexIsValid, SolidSyslogPoolAllocator_FreeIfInUse and
FileBlockDevice_CleanupAtIndex) so valid pool frees still occur and the
SOLIDSYSLOG_ERROR_MSG_FILEBLOCKDEVICE_UNKNOWN_DESTROY warning is only emitted
for truly unknown/unmanaged handles.
Owner
Author
There was a problem hiding this comment.
Declined — keeping the warning. The Create-time error already flags a misconfigured pool with SOLIDSYSLOG_ERROR_MSG_FILEBLOCKDEVICE_POOL_EXHAUSTED; a Destroy-time warning on the same bad setup makes the misuse more discoverable, not less. Suppressing it would hide a real signal when an integrator destroys something they didn't actually own. No crash risk either way — destroying the null-object handle is safe; we just won't pretend the call was correct.
Rationale captured in DEVLOG (commit 84f0edf).
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the dual-signal pattern (Create-time POOL_EXHAUSTED + Destroy-time UNKNOWN_DESTROY) deliberately amplifies the misuse signal for integrators with undersized pools. That's a reasonable design choice. I'll stand down on this.
( ￣ε￣)
✏️ Learnings added
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 DavidCozens/solid-syslog, when a pool-backed Create (e.g., SolidSyslogFileBlockDevice_Create, SolidSyslogStreamSender_Create) falls back to a shared null-object on pool exhaustion, the corresponding Destroy function intentionally still emits an UNKNOWN_DESTROY warning when given that null-object handle. This is a deliberate dual-signal design: Create-time POOL_EXHAUSTED + Destroy-time UNKNOWN_DESTROY together make pool misconfiguration maximally discoverable. Do not suggest suppressing the Destroy-time warning for the null-object fallback handle.
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.
Comment on lines
+42
to
+51
| void SolidSyslogStreamSender_Destroy(struct SolidSyslogSender* base) | ||
| { | ||
| size_t index = StreamSender_IndexFromHandle(base); | ||
| bool released = | ||
| SolidSyslogPoolAllocator_IndexIsValid(&StreamSender_Allocator, index) && | ||
| SolidSyslogPoolAllocator_FreeIfInUse(&StreamSender_Allocator, index, StreamSender_CleanupAtIndex, NULL); | ||
| if (!released) | ||
| { | ||
| SolidSyslog_Error(SOLIDSYSLOG_SEVERITY_WARNING, SOLIDSYSLOG_ERROR_MSG_STREAMSENDER_UNKNOWN_DESTROY); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid warning on destroy of the overflow fallback sender.
When Create overflows (Line 28/Line 37), it returns SolidSyslogNullSender_Get(). Destroying that valid fallback currently always logs SOLIDSYSLOG_ERROR_MSG_STREAMSENDER_UNKNOWN_DESTROY (Line 50), which is noisy and misleading.
Proposed fix
void SolidSyslogStreamSender_Destroy(struct SolidSyslogSender* base)
{
+ if (base == SolidSyslogNullSender_Get())
+ {
+ return;
+ }
+
size_t index = StreamSender_IndexFromHandle(base);
bool released =
SolidSyslogPoolAllocator_IndexIsValid(&StreamSender_Allocator, index) &&
SolidSyslogPoolAllocator_FreeIfInUse(&StreamSender_Allocator, index, StreamSender_CleanupAtIndex, NULL);🤖 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 `@Core/Source/SolidSyslogStreamSenderStatic.c` around lines 42 - 51,
SolidSyslogStreamSender_Destroy currently logs a warning when called with the
overflow fallback/null sender returned by Create; update the function to detect
the null/fallback sender (compare the passed base pointer to
SolidSyslogNullSender_Get() or use an equivalent "is null sender" check) and
silently return without logging; keep the existing allocator/index checks
(StreamSender_IndexFromHandle and
SolidSyslogPoolAllocator_IndexIsValid/FreeIfInUse) for normal senders, but skip
the warning path when base is the null/fallback sender.
Owner
Author
There was a problem hiding this comment.
Declined for the same reason as the FileBlockDevice short-circuit above — the noisy warning on Destroy is correct signal, not noise. A bad-setup Create already emits ERR (SOLIDSYSLOG_ERROR_MSG_STREAMSENDER_POOL_EXHAUSTED); the WARNING on Destroy points at the same misconfiguration from the other side of the lifecycle. The null-object vtable means there's no crash risk — destroying it is safe, we just don't pretend it was a real handle.
Also note: the BDD CI failures on this PR were caused by exactly the silent-fallback path this comment proposes to make even quieter. Bumping SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE default 1→2 in 84f0edf addresses the actual root cause (pool too small for typical TCP+TLS-fallback wiring); keeping the Destroy warning helps surface the next case.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the Create ERR + Destroy WARNING pair is a deliberate two-sided diagnostic for pool misconfiguration, not redundant noise. The null-object vtable makes the destroy safe, but the warning intentionally surfaces the misconfiguration from the lifecycle's exit side. Makes sense, and good context on the BDD CI failure being the motivator for the SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE default bump rather than silencing the signal. I'll withdraw this suggestion.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 402
File: Core/Source/SolidSyslogStreamSenderStatic.c:42-51
Timestamp: 2026-05-19T06:57:24.805Z
Learning: In DavidCozens/solid-syslog, the WARNING emitted by `SolidSyslogStreamSender_Destroy` (and `SolidSyslogFileBlockDevice_Destroy`) when called with a handle not issued by the pool (including the null-object fallback returned on pool exhaustion) is intentional and must NOT be short-circuited. The Create ERROR (pool exhaustion) and Destroy WARNING together form a correlated two-sided diagnostic for pool misconfiguration. The null-object vtable ensures crash safety; the warning ensures the misconfiguration is visible from both sides of the lifecycle. Do not suggest silencing either signal.
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.
Part A's CI surfaced two red gates after push:
- analyze-iwyu: five include adjustments. Two Static.c files were
pulling in SolidSyslogBufferDefinition.h when only the pointer was
used (the full definition arrives transitively via the Private.h);
FileBlockDevice.c carried an unused SolidSyslogMacros.h and was
missing <stdint.h> for the uint8_t cast in _FormatBlockFilename;
StreamSender.c was using SolidSyslogStreamSenderConfig without a
direct include of its public header; NullBufferTest.cpp used size_t
without <stddef.h>. All fixed; iwyu now clean.
- bdd-linux-syslog-ng / bdd-windows-otel: 4+4 TLS/mTLS scenarios
silently dropping ("received 0 of 1 messages"). Root cause: the
Linux and Windows BDD targets each wire two StreamSenders into a
SwitchingSender (plain TCP at one branch, TLS at another) but
SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE defaulted to 1 — the second
Create returned SolidSyslogNullSender_Get() and dropped on Send.
Bumped the default to 2; the previous tunable comment ("almost
all integrators wire a single stream-framed sender … as one
branch of a SwitchingSender") was too optimistic. A
TCP-with-TLS-fallback wiring is a realistic shape and one extra
pool slot (~64B/64-bit) is trivial against silent message drop.
Comment rewritten to match.
Also absorbs two CodeRabbit doc nits — CLAUDE.md BrE consistency on
"synchronise"; DEVLOG typo "user-after-destroy" → "use-after-destroy".
CodeRabbit also proposed two production-code suggestions to silence
the UNKNOWN_DESTROY warning when the caller passes the Create-time
null-object fallback back through _Destroy (FileBlockDevice and
StreamSender). Declined — the warning is correct signal: the
Create-time error already flagged a misconfigured pool, and a
Destroy-time warning on the same bad setup makes the misuse *more*
discoverable. Suppressing it would hide a real signal when the
integrator destroys something they didn't actually own. No crash
risk either way — the null-object vtable is safe to destroy.
DEVLOG updated with rationale on the same.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1182 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This was referenced May 19, 2026
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
First half of S11.05 (#401). Migrates the two public storage-cast classes
(StreamSender, FileBlockDevice) onto
SolidSyslogPoolAllocator, ships twonew shared GoF nulls that the migrations depend on, and retrofits every
previously-migrated class so use-after-destroy is a safe no-op rather than
a NULL-fn-pointer crash.
The remaining BlockStore composition migration (RecordStore + BlockSequence
in a follow-up PR; see the scope-split comment on S11.05: Sweep — Core storage-cast classes onto PoolAllocator #401.
Refs: #401, parent epic #29.
Change Description
Four commits, three architectural threads:
Public-class migrations (the headline E11 work)
refactor: S11.05 migrate StreamSender onto PoolAllocatorandfeat: S11.05 migrate FileBlockDevice onto PoolAllocator. Both adopt thecanonical 3-TU split (
<Class>.c+<Class>Private.h+<Class>Static.c),delete the public
SolidSyslog<Class>Storagetypedef andSOLIDSYSLOG_<*>_*_SIZEenum from the audience headers, and drop thestorageparameter from_Create. Pool size tunables ship asSOLIDSYSLOG_STREAM_SENDER_POOL_SIZEandSOLIDSYSLOG_FILE_BLOCK_DEVICE_POOL_SIZE(default 1; floor 1 enforcedat compile time). Pool exhaustion resolves to the shared NullSender /
NullBlockDevice; bad-config errors emit
SolidSyslog_Error(ERROR, ...).Callers updated: BDD targets (Linux / Windows / FreeRTOS / two TLS
common variants), the relevant test fixtures, the public-header
audience-table rows in
CLAUDE.md.Use-after-destroy crash-safety
fix: S11.05 install null-object vtable on _Cleanup for crash-safety.A separate commit because it touches eight classes — six S11.04
migrations plus the S11.01 pilot plus my new StreamSender. The previous
shape NULL'd the abstract-base function pointers in
_Cleanup, socalling Send / Disconnect / Write / Read / Format through a stale
handle was an immediate crash. The pre-E11
*self = DESTROYED_INSTANCEpattern had the same hazard.
Replaced by
*base = *SolidSyslogNull<Family>_Get();— copy the sharedGoF null's vtable into the slot's abstract base. Derived fields are
TU-private and the next
_Initialiseoverwrites them on slot reuse, sono need to wipe them on Cleanup. UdpSender and StreamSender call their
own
Disconnectfirst (still need the live config); the rest is aone-liner. One
UseAfterDestroyIsCrashSafeVia…Vtabletest per classexercises Send / Format / Write / Read through a stale handle.
Two new shared GoF nulls
SolidSyslogNullBlockDeviceandSolidSyslogNullBuffership as public`Get`-only objects joining the NullSender / NullSd / NullStore family.
NullBlockDevice methods all return false / 0; NullBuffer.Write swallows
and NullBuffer.Read returns false with bytesRead=0. The same instance
serves two consumers in this PR: the pool-exhaustion fallback on
<Class>_Createand the vtable target on<Class>_Cleanup.PassthroughBuffer + CircularBuffer also retrofit to use the new
NullBuffer— their class-privateFallbackstatic (added in thecrash-safety commit as an interim shape) is deleted entirely.
FileBlockDevice arrives fresh with no class-private fallback at all.
Test Evidence
the crash-safety retrofit + StreamSender + FileBlockDevice)
(`FillingPoolThenOverflowReturnsDistinctFallback` per the
S11.05 acceptance criterion)
1178 in the Core executable (was 1155 on `main`).
the `SOLIDSYSLOG_USER_TUNABLES_FILE` override mechanism, for both
StreamSender and FileBlockDevice. Full suite green at the bumped
pool sizes.
`feedback_verify_in_freertos_host_image`. All eight test binaries
green with `FREERTOS_KERNEL_PATH=/opt/freertos/kernel` set:
Core (1178), FatFs (28), Cmsdk (15), FreeRtosDatagram (21),
FreeRtosMutex (5), FreeRtosStaticResolver (10), FreeRtosSysUpTime
(4), FreeRtosTcpStream (37). 1298 tests total.
MISRA
`cppcheck-misra` count: 92 (was 98 on `main`). Six findings touched:
Five evaporated —
`Fallback_*` statics are gone)
DESTROYED_INSTANCE deleted by the migration)
Two new (same shape as existing main findings) —
Both fire on the file-scope `instance` variable that every Null*
class uses; NullSender / NullSd / NullStore already have the same
8.9 finding on `main`. The new shared nulls inherit the established
pattern.
Zero new architectural findings; the cleanup is strictly net-positive.
Local gates run
All green: `debug`, `clang-debug`, `sanitize`, `coverage` (99.6%
line, 99.1% function), `tidy`, `cppcheck` (clean), `cppcheck-misra`
(diff above), `clang-format` (dry-run clean), `iwyu`. Plus the
cpputest-freertos cross-check above.
Areas Affected
(`SolidSyslogNullBlockDevice.h`, `SolidSyslogNullBuffer.h`); the
`SolidSyslogStreamSender.h` and `SolidSyslogFileBlockDevice.h`
audience headers lose their `Storage` typedef + `_SIZE` enum and
the `_Create` storage parameter; new pool-size tunables in
`SolidSyslogTunablesDefaults.h`.
3-TU splits per migrated public class; the eight `_Cleanup`
bodies across the previously-migrated classes; the new error-message
symbols.
use-after-destroy tests; two new pool TEST_GROUPs.
local declarations and the storage argument to `_Create`.
Out of scope (for part B / future)
BlockStore migrations and the embedded-struct → pointer rewrite).
Follow-up PR.
`_Destroy`). Belongs with the broader Null* family but the buffer
tests' Mutex lifecycle needs a careful read before flipping.
but explicitly deferred to E12 — not appropriate inside a
storage-cast sweep PR.
Closes #401 is NOT added — this PR delivers part A only; #401
stays open until part B merges.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests