feat: S11.06 POSIX adapters onto PoolAllocator#407
Merged
Conversation
Shared GoF null object for the Datagram vtable, used as the fallback when PosixDatagram_Create (S11.06) or FreeRtosDatagram_Create (S11.08) exhaust their respective pools or hit bad config. - Open returns true so callers do not loop on a false from the no-op. - SendTo returns SENT to drop on the floor — UdpSender treats the message as delivered and the Store does not accumulate undeliverables. Mirrors NullSender_Send's contract. - MaxPayload returns SOLIDSYSLOG_UDP_IPV6_SAFE_PAYLOAD so any direct caller (e.g. the OVERSIZE retry path) sees a sensible default. - Close is a no-op. Compiled under the same network-backend gate as SolidSyslogDatagram.c in Core/Source/CMakeLists.txt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shared GoF null object for the Stream vtable, used as the fallback
when PosixTcpStream_Create (S11.06), WinsockTcpStream_Create (S11.07),
FreeRtosTcpStream_Create (S11.08), or TlsStream_Create (S11.09)
exhaust their respective pools or hit bad config.
- Open returns true so callers do not loop on a no-op fail.
- Send returns true to drop on the floor — StreamSender treats the
message as delivered and the Store does not accumulate undeliverables.
Mirrors NullSender_Send.
- Read returns 0 ("would-block, nothing available") rather than < 0,
so the caller does not flag a broken connection and tear it down.
- Close is a no-op.
NullStream lives in the unconditional Core/Source section because
SolidSyslogStream.c (its base dispatcher) is unconditional.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shared GoF null object for the File vtable, used as the fallback when PosixFile_Create (S11.06), WindowsFile_Create (S11.07), or FatFsFile_Create (S11.09) exhaust their respective pools or hit bad config. Semantics — chosen so a consumer treating NullFile as a non-functional file degrades cleanly: - Open / IsOpen / Read / Exists return false — presents consistently as a closed, empty, non-existent file. - Write / Delete return true — the BlockStore-side caller treats the bytes as persisted (drop-on-the-floor; mirrors NullSender_Send) and delete-of-nonexistent is vacuously success. - Close / SeekTo / Truncate are no-ops. - Size returns 0. In practice the chain is already broken (BlockStore is on NullStore) by the time NullFile is reached, but the semantics here are defensible in isolation too. Lives in the unconditional Core/Source section because SolidSyslogFile.c is unconditional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shared GoF null object for the Resolver vtable, used as the fallback
when GetAddrInfoResolver_Create (S11.06) or
FreeRtosStaticResolver_Create (S11.08) exhaust their respective pools
or hit bad config.
Single vtable method:
- Resolve returns false ("could not resolve") so the caller's existing
unresolved-host error path runs naturally. In the pool-exhausted
fallback case the matching Sender is already on NullSender, so the
send chain is broken end-to-end.
Compiled under the same network-backend gate as SolidSyslogResolver.c.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The newer GoF nulls (NullSender, NullStore, NullSd, NullSecurityPolicy, NullBuffer, NullBlockDevice, plus this story's NullDatagram, NullStream, NullFile, NullResolver) all use _Get(void) — single static instance, vtable initialised at file scope, no lifecycle. NullMutex was the last holdout still using _Create / _Destroy. Mutex semantics here are stateless (no-op Lock/Unlock), so the Create-time vtable assignment + Destroy-time NULL-out it carried served no purpose. Mechanical: drop _Destroy, replace _Create with _Get returning the file-scope-initialised instance, update all four caller files (NullMutexTest, CircularBufferTest, BlockStoreDrainOrderingTest, SolidSyslogTest) and the CLAUDE.md audience-table row. Pure shape refactor — existing behaviour preserved end-to-end, full suite green. NullMutexTest gains GetReturnsSameInstance to drive the singleton contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogPosixMutex.c — vtable + Initialise/Cleanup; no allocation. - SolidSyslogPosixMutexPrivate.h — concrete struct + private signatures, TU-internal. - SolidSyslogPosixMutexStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex bridge. Public API change: Before: SolidSyslogPosixMutex_Create(SolidSyslogPosixMutexStorage*) After: SolidSyslogPosixMutex_Create(void) Public Storage type and SOLIDSYSLOG_POSIX_MUTEX_SIZE enum deleted — caller-supplied storage replaced by the internal pool. _Destroy(base) shape preserved from S11.04. Pool size tunable: SOLIDSYSLOG_POSIX_MUTEX_POOL_SIZE, default 1U. Override via SOLIDSYSLOG_USER_TUNABLES_FILE per the existing mechanism. Pool exhaustion returns the shared SolidSyslogNullMutex with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown/stale handle reports WARNING. Same contract as the S11.02 CircularBuffer pilot. Cleanup additionally overwrites the abstract base with NullMutex's vtable so post-destroy Lock/Unlock are safe no-ops rather than NULL function-pointer crashes (mirrors CircularBuffer_Cleanup). Existing tests carry forward as the regression net minus HandleEqualsStorageAddress (no longer meaningful — handle no longer equals injected storage). New 9-test SolidSyslogPosixMutexPool group covers the pool contract: fallback distinctness, error reporting, ConfigLock acquisition pattern (once per probed slot), and stale / unknown destroy reporting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogPosixDatagram.c — vtable + Initialise/Cleanup; no allocation. File-scope `instance` removed. - SolidSyslogPosixDatagramPrivate.h — concrete struct + private signatures, TU-internal. - SolidSyslogPosixDatagramStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. Public API change: Before: void SolidSyslogPosixDatagram_Destroy(void) After: void SolidSyslogPosixDatagram_Destroy(struct SolidSyslogDatagram*) Pool size tunable: SOLIDSYSLOG_POSIX_DATAGRAM_POOL_SIZE, default 1U. Override via SOLIDSYSLOG_USER_TUNABLES_FILE. Pool exhaustion returns the shared SolidSyslogNullDatagram with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown / stale handle reports WARNING. Same contract as PosixMutex. PosixDatagram_Initialise now explicitly resets Fd = INVALID_FD and Connected = false rather than relying on the file-scope designated initialiser. Behaviour-preserving fix that surfaced naturally with the pool migration (slots are zero-initialised, where Fd=0 would be a valid file descriptor). Cleanup additionally overwrites the abstract base with NullDatagram's vtable so post-destroy Open/SendTo/MaxPayload/Close are safe no-ops. Existing 28 tests carry forward as the regression net. Teardown updated to pass the datagram handle in three caller files: SolidSyslogUdpSenderTest.cpp, BddTargetServiceThreadTest.cpp, Bdd/Targets/Linux/main.c. New 9-test SolidSyslogPosixDatagramPool group covers the pool contract: fallback distinctness, error reporting, ConfigLock acquisition pattern, and stale / unknown destroy reporting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogGetAddrInfoResolver.c — vtable + Initialise/Cleanup. - SolidSyslogGetAddrInfoResolverPrivate.h — concrete struct + private signatures, TU-internal. - SolidSyslogGetAddrInfoResolverStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. Public API change: Before: void SolidSyslogGetAddrInfoResolver_Destroy(void) After: void SolidSyslogGetAddrInfoResolver_Destroy(struct SolidSyslogResolver*) Pool size tunable: SOLIDSYSLOG_GETADDRINFO_RESOLVER_POOL_SIZE, default 1U. The class is effectively stateless today — the pool slot carries the vtable holder. Migrating for shape consistency with the other Resolver/Datagram/Stream impls that *will* carry state (FreeRtosStaticResolver in S11.08). Pool exhaustion returns the shared SolidSyslogNullResolver with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown / stale handle reports WARNING. Cleanup overwrites the abstract base with NullResolver's vtable so post-destroy Resolve is a safe no-op (returns false). Callers updated to pass the resolver handle to _Destroy: - Tests/SolidSyslogGetAddrInfoResolverTest.cpp - Tests/SolidSyslogUdpSenderTest.cpp - Tests/SolidSyslogStreamSenderTest.cpp (five teardowns) - Tests/Bdd/Targets/BddTargetServiceThreadTest.cpp - Bdd/Targets/Linux/main.c The StreamSenderConfig group's CreateSender previously held the resolver in a local; promoted to a fixture member to keep the teardown reach. main.c's CreateSender similarly captures into a file-scope sharedResolver pointer for DestroySender. New 9-test SolidSyslogGetAddrInfoResolverPool group covers the pool contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogPosixFile.c — vtable + Initialise/Cleanup; no allocation. - SolidSyslogPosixFilePrivate.h — concrete struct + private signatures, TU-internal. - SolidSyslogPosixFileStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. Public API change: Before: SolidSyslogPosixFile_Create(SolidSyslogPosixFileStorage*) After: SolidSyslogPosixFile_Create(void) Public Storage type and SOLIDSYSLOG_POSIX_FILE_SIZE enum deleted — caller-supplied storage replaced by the internal pool. Pool size tunable: SOLIDSYSLOG_POSIX_FILE_POOL_SIZE, default 1U. Override via SOLIDSYSLOG_USER_TUNABLES_FILE. Pool exhaustion returns the shared SolidSyslogNullFile with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown / stale handle reports WARNING. The DEFAULT_INSTANCE / DESTROYED_INSTANCE static templates the old storage-cast shape used are dropped — Initialise sets every vtable field explicitly, and Cleanup overwrites the abstract base with NullFile's vtable for the safe-no-op post-destroy contract. Callers updated: - Tests/SolidSyslogPosixFileTest.cpp — drop storage local, plus remove CreateReturnsHandleInsideCallerSuppliedStorage (handle no longer equals storage by construction). - Tests/SolidSyslogBlockStorePosixTest.cpp — drop fileStorage local. - Bdd/Targets/Linux/main.c — drop fileStorage static. New 9-test SolidSyslogPosixFilePool group covers the pool contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogPosixTcpStream.c — vtable + Initialise/Cleanup; no allocation. Static DEFAULT_INSTANCE / DESTROYED_INSTANCE templates dropped. - SolidSyslogPosixTcpStreamPrivate.h — concrete struct + private signatures, TU-internal. - SolidSyslogPosixTcpStreamStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. Public API change: Before: SolidSyslogPosixTcpStream_Create(SolidSyslogPosixTcpStreamStorage*) After: SolidSyslogPosixTcpStream_Create(void) Public Storage type and SOLIDSYSLOG_POSIX_TCP_STREAM_SIZE enum deleted. Pool size tunable: SOLIDSYSLOG_POSIX_TCP_STREAM_POOL_SIZE, default 2U (not 1U). The Linux BDD target wires two stream-framed senders behind a SwitchingSender — plain TCP plus TLS that wraps another underlying PosixTcpStream as its transport — so default 1 would silently fall back to NullStream on the second Create. Matches the SOLIDSYSLOG_STREAM_SENDER_POOL_SIZE default bumped to 2 in S11.05 for the same reason. Pool exhaustion returns the shared SolidSyslogNullStream with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown / stale handle reports WARNING. Cleanup overwrites the abstract base with NullStream's vtable for the safe-no-op post-destroy contract. The existing PosixTcpStream connect semantics — non-blocking start, select-bounded wait via CONNECT_TIMEOUT_MICROSECONDS, deferred SO_ERROR read, plus TCP_NODELAY + keepalive + TCP_USER_TIMEOUT — are preserved verbatim. The behavior of the Open() retry loop after a failed Connect (close-on-failure, INVALID_FD reset) is identical to the pre-migration code; only the Create/Destroy lifecycle moves into the pool. Callers updated: - Tests/SolidSyslogPosixTcpStreamTest.cpp — drop storage local, plus remove CreateReturnsHandleInsideCallerSuppliedStorage. - Tests/SolidSyslogStreamSenderTest.cpp — drop streamStorage locals in five TEST_GROUPs. - Bdd/Targets/Linux/main.c — drop plainTcpStreamStorage static. - Bdd/Targets/Common/BddTargetTlsSender_OpenSsl_PosixTcp.c — drop underlyingStreamStorage static. New 9-test SolidSyslogPosixTcpStreamPool group covers the pool contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adopts the canonical 3-TU split + SolidSyslogPoolAllocator shape: - SolidSyslogPosixMessageQueueBuffer.c — vtable + Initialise/Cleanup; no allocation. File-scope `instance` and QueueName helper now per-slot. - SolidSyslogPosixMessageQueueBufferPrivate.h — concrete struct + private signatures, TU-internal. Carries the per-instance mq_t, Formatter name storage, and MaxMessageSize. - SolidSyslogPosixMessageQueueBufferStatic.c — library-internal pool, Create/Destroy, IndexFromHandle, CleanupAtIndex. Public API change: Before: void SolidSyslogPosixMessageQueueBuffer_Destroy(void) After: void SolidSyslogPosixMessageQueueBuffer_Destroy(struct SolidSyslogBuffer*) `_Create(maxMessageSize, maxMessages)` keeps the positional pair unchanged. Pool size tunable: SOLIDSYSLOG_POSIX_MESSAGE_QUEUE_BUFFER_POOL_SIZE, default 1U. The queue name derives from the process ID, so two slots in the same process would race onto the same `/solidsyslog_<pid>` name — an integrator needing N > 1 must additionally distinguish the names. The tunable header comment + the audience-table row + a Static.c comment all flag this limit; the contract today is one MQ buffer per process. Pool exhaustion returns the shared SolidSyslogNullBuffer with an ERROR-severity SolidSyslog_Error report. Destroy of an unknown / stale handle reports WARNING. Cleanup overwrites the abstract base with NullBuffer's vtable for safe-no-op post-destroy. Callers updated: - Tests/SolidSyslogPosixMessageQueueBufferTest.cpp — handle-based teardown. - Tests/Bdd/Targets/BddTargetServiceThreadTest.cpp — handle-based teardown. - Bdd/Targets/Linux/main.c — handle-based teardown. New 9-test SolidSyslogPosixMessageQueueBufferPool group covers the pool contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes surfaced by running the local gates over the full migrated tree: - **iwyu**: add explicit `#include "SolidSyslogStream.h"` / `SolidSyslogDatagram.h` in the Null*.c files (transitively pulled in via the Definition.h today but iwyu wants explicit); add a forward `struct SolidSyslog<Base>;` declaration to each *Static.c (matches the established pattern from CircularBufferStatic.c); add explicit `<stddef.h>` to the NullDatagramTest for `size_t`. - **cppcheck**: suppress two false positives per Pool TEST_GROUP fixture across all six new pool test groups — `constVariable` on the `pooled[]` array (cppcheck doesn't see test bodies assigning into the array) and `knownConditionTrueFalse` on the teardown's `overflow != nullptr` guard (same reason). Existing Pool test groups (CircularBuffer, StreamSender, etc.) pass without these suppressions; cppcheck's CTU analysis is order-sensitive and inconsistent — the new test files happen to be analysed in an order that surfaces it. - **tidy**: add a per-call NOLINT to PosixMessageQueueBuffer_Initialise for `bugprone-easily-swappable-parameters` (mirrors the existing NOLINT on the public `_Create` signature it carries forward). - **dead code**: drop an unused `struct InitContext` left over from an early draft of MessageQueueBufferStatic.c (caught by tidy's readability-identifier-naming). All gates green after these fixes: debug + clang-debug + sanitize + coverage + tidy + cppcheck + iwyu + format. Coverage 100% line on every new file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 (10)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMigrate POSIX adapters from storage/singleton lifecycles to pool-backed Create/Destroy; add null-object singletons; introduce per-class pool tunables and error messages; update CMake, call sites, and tests to use handle-based lifecycles; add pool tests and DEVLOG/CLAUDE updates. ChangesPool Allocator Adapter Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1262 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
🧹 Nitpick comments (1)
Tests/SolidSyslogGetAddrInfoResolverTest.cpp (1)
132-144: ⚡ Quick winConsolidate
CHECK_IS_FALLBACKinto shared test utilities.This macro is duplicated across multiple pool test files; moving it to a shared test helper will avoid drift and reduce maintenance overhead.
As per coding guidelines, "In test code, use
CHECK_*macros for repeated assertion shapes; wrap with NOLINTBEGIN/NOLINTEND and usedo { ... } while (0)for safe single-statement use".🤖 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/SolidSyslogGetAddrInfoResolverTest.cpp` around lines 132 - 144, Extract the CHECK_IS_FALLBACK macro into a shared test helper header (e.g., declare the macro in a new or existing test utilities header) and remove the duplicate macro definitions from individual pool test files; update those tests to include the shared header and keep the existing NOLINTBEGIN/NOLINTEND and do { ... } while (0) wrapper as-is so usage of CHECK_IS_FALLBACK(handle, pool) remains unchanged; ensure any file that used the old macro includes the new helper and run tests to confirm no symbol collisions.
🤖 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 `@Core/Source/SolidSyslogNullMutex.c`:
- Around line 8-11: The file-scope static variable named "instance" does not
follow the project's file-scope static naming convention; rename it to a
conventionally compliant identifier (e.g., SolidSyslogNullMutex_INSTANCE or
SolidSyslogNullMutex_Instance) and update its declaration and any references so
the static struct SolidSyslogMutex variable that assigns .Lock = NullMutex_Lock
and .Unlock = NullMutex_Unlock uses the new name consistently (ensure references
to "instance" are replaced throughout this translation unit).
In `@Platform/Posix/Source/SolidSyslogPosixMessageQueueBufferStatic.c`:
- Around line 16-20: The pid-derived queue name "/solidsyslog_<pid>" currently
lacks a per-slot discriminator so multiple pool entries in one process can
collide; update the queue-name construction and the initialization code that
currently uses the pid-only name (the block around the comment and the init at
lines ~33-40) to include a unique token such as the pool slot `index` (or
another per-instance identifier) so each pool slot maps to a distinct queue name
and lifecycle isolation is preserved.
In `@Platform/Posix/Source/SolidSyslogPosixMutex.c`:
- Around line 15-30: PosixMutex_Initialise currently ignores the return value of
pthread_mutex_init and always installs PosixMutex_Lock/Unlock, so if init fails
the mutex is uninitialized; change PosixMutex_Initialise to check
pthread_mutex_init's return and on failure set *base =
*SolidSyslogNullMutex_Get() (do not assign the Posix vtable or touch
self->Mutex), and record a flag (e.g. add or reuse a field like
self->Initialized) or rely on comparing the vtable to NullMutex to indicate
success; then update PosixMutex_Cleanup to only call
pthread_mutex_destroy(&self->Mutex) if init succeeded (i.e., when the Posix
vtable is installed/Initialized is true) and otherwise skip destroy—use the
symbols PosixMutex_Initialise, PosixMutex_Cleanup, SolidSyslogPosixMutex,
pthread_mutex_init, pthread_mutex_destroy and SolidSyslogNullMutex_Get to locate
and implement the changes.
---
Nitpick comments:
In `@Tests/SolidSyslogGetAddrInfoResolverTest.cpp`:
- Around line 132-144: Extract the CHECK_IS_FALLBACK macro into a shared test
helper header (e.g., declare the macro in a new or existing test utilities
header) and remove the duplicate macro definitions from individual pool test
files; update those tests to include the shared header and keep the existing
NOLINTBEGIN/NOLINTEND and do { ... } while (0) wrapper as-is so usage of
CHECK_IS_FALLBACK(handle, pool) remains unchanged; ensure any file that used the
old macro includes the new helper and run tests to confirm no symbol collisions.
🪄 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: 3691c156-e321-4efd-878d-737dd5e9246e
📒 Files selected for processing (61)
Bdd/Targets/Common/BddTargetTlsSender_OpenSsl_PosixTcp.cBdd/Targets/Linux/main.cCLAUDE.mdCore/Interface/SolidSyslogNullDatagram.hCore/Interface/SolidSyslogNullFile.hCore/Interface/SolidSyslogNullMutex.hCore/Interface/SolidSyslogNullResolver.hCore/Interface/SolidSyslogNullStream.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/CMakeLists.txtCore/Source/SolidSyslogErrorMessages.hCore/Source/SolidSyslogNullDatagram.cCore/Source/SolidSyslogNullFile.cCore/Source/SolidSyslogNullMutex.cCore/Source/SolidSyslogNullResolver.cCore/Source/SolidSyslogNullStream.cDEVLOG.mdPlatform/Posix/CMakeLists.txtPlatform/Posix/Interface/SolidSyslogGetAddrInfoResolver.hPlatform/Posix/Interface/SolidSyslogPosixDatagram.hPlatform/Posix/Interface/SolidSyslogPosixFile.hPlatform/Posix/Interface/SolidSyslogPosixMessageQueueBuffer.hPlatform/Posix/Interface/SolidSyslogPosixMutex.hPlatform/Posix/Interface/SolidSyslogPosixTcpStream.hPlatform/Posix/Source/SolidSyslogGetAddrInfoResolver.cPlatform/Posix/Source/SolidSyslogGetAddrInfoResolverPrivate.hPlatform/Posix/Source/SolidSyslogGetAddrInfoResolverStatic.cPlatform/Posix/Source/SolidSyslogPosixDatagram.cPlatform/Posix/Source/SolidSyslogPosixDatagramPrivate.hPlatform/Posix/Source/SolidSyslogPosixDatagramStatic.cPlatform/Posix/Source/SolidSyslogPosixFile.cPlatform/Posix/Source/SolidSyslogPosixFilePrivate.hPlatform/Posix/Source/SolidSyslogPosixFileStatic.cPlatform/Posix/Source/SolidSyslogPosixMessageQueueBuffer.cPlatform/Posix/Source/SolidSyslogPosixMessageQueueBufferPrivate.hPlatform/Posix/Source/SolidSyslogPosixMessageQueueBufferStatic.cPlatform/Posix/Source/SolidSyslogPosixMutex.cPlatform/Posix/Source/SolidSyslogPosixMutexPrivate.hPlatform/Posix/Source/SolidSyslogPosixMutexStatic.cPlatform/Posix/Source/SolidSyslogPosixTcpStream.cPlatform/Posix/Source/SolidSyslogPosixTcpStreamPrivate.hPlatform/Posix/Source/SolidSyslogPosixTcpStreamStatic.cTests/Bdd/Targets/BddTargetServiceThreadTest.cppTests/CMakeLists.txtTests/SolidSyslogBlockStoreDrainOrderingTest.cppTests/SolidSyslogBlockStorePosixTest.cppTests/SolidSyslogCircularBufferTest.cppTests/SolidSyslogGetAddrInfoResolverTest.cppTests/SolidSyslogNullDatagramTest.cppTests/SolidSyslogNullFileTest.cppTests/SolidSyslogNullMutexTest.cppTests/SolidSyslogNullResolverTest.cppTests/SolidSyslogNullStreamTest.cppTests/SolidSyslogPosixDatagramTest.cppTests/SolidSyslogPosixFileTest.cppTests/SolidSyslogPosixMessageQueueBufferTest.cppTests/SolidSyslogPosixMutexTest.cppTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogStreamSenderTest.cppTests/SolidSyslogTest.cppTests/SolidSyslogUdpSenderTest.cpp
Comment on lines
+8
to
+11
| static struct SolidSyslogMutex instance = { | ||
| .Lock = NullMutex_Lock, | ||
| .Unlock = NullMutex_Unlock, | ||
| }; |
Contributor
There was a problem hiding this comment.
Rename the file-scope static to match project naming rules.
instance does not follow the required file-scope static naming convention in this repo.
Suggested rename
-static struct SolidSyslogMutex instance = {
+static struct SolidSyslogMutex NullMutex_Instance = {
.Lock = NullMutex_Lock,
.Unlock = NullMutex_Unlock,
};
@@
- return &instance;
+ return &NullMutex_Instance;As per coding guidelines: **/*.{c,h,cpp,hpp} requires file-scope statics to use Class_Function / CLASS_SCREAMING_SNAKE naming.
🤖 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/SolidSyslogNullMutex.c` around lines 8 - 11, The file-scope
static variable named "instance" does not follow the project's file-scope static
naming convention; rename it to a conventionally compliant identifier (e.g.,
SolidSyslogNullMutex_INSTANCE or SolidSyslogNullMutex_Instance) and update its
declaration and any references so the static struct SolidSyslogMutex variable
that assigns .Lock = NullMutex_Lock and .Unlock = NullMutex_Unlock uses the new
name consistently (ensure references to "instance" are replaced throughout this
translation unit).
Owner
Author
There was a problem hiding this comment.
Deferred — the instance identifier is the convention across all 11 GoF nulls in the tree (NullBuffer / NullBlockDevice / NullDatagram / NullFile / NullMutex / NullResolver / NullSd / NullSecurityPolicy / NullSender / NullStore / NullStream). Per the project_naming_sweep_deferred memory the right move is to match the dominant local pattern and let a dedicated naming sweep do the tree-wide rename. Fixing it for NullMutex alone would create the inconsistency the lint is trying to prevent.
Contributor
There was a problem hiding this comment.
@DavidCozens, that makes complete sense — matching the dominant local pattern across all 11 GoF nulls is the right call. I'll stop flagging instance as a naming violation in these null-object translation units and leave any tree-wide rename to a dedicated sweep.
🐇 ✿
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 407
File: Core/Source/SolidSyslogNullMutex.c:8-11
Timestamp: 2026-05-20T06:36:51.024Z
Learning: In DavidCozens/solid-syslog, the file-scope static singleton variable in all GoF null-object translation units (NullBuffer, NullBlockDevice, NullDatagram, NullFile, NullMutex, NullResolver, NullSd, NullSecurityPolicy, NullSender, NullStore, NullStream) is intentionally named `instance` — this is the established project-wide convention for the GoF null family, not a naming-convention violation. Do not flag `instance` as a non-conformant identifier in any of these null-object .c files. Any tree-wide rename is deferred to a dedicated naming sweep.
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.
Three classes of fixes folded into one commit:
**MSVC /W4 /WX (build-windows-msvc):** the generic `CHECK_EQUAL` macro
triggers C4127 ("conditional expression is constant") under MSVC's
warnings-as-errors at four sites. Other Null tests in the codebase avoid
it by using the specialized `LONGS_EQUAL` / `UNSIGNED_LONGS_EQUAL`
macros instead — convention I missed. Switched the four sites in
NullDatagramTest / NullFileTest / NullStreamTest to match.
**clang-tidy (analyze-tidy):**
`Tests/SolidSyslogPosixMessageQueueBufferTest.cpp:178` MakeBuffer
fixture helper marked `static` — it references no instance state, so
`readability-convert-member-functions-to-static` fires. Other test
files' MakeBuffer keeps non-static because they reference fixture data.
**CodeRabbit feedback (PR #407):**
- **PosixMutex pthread_mutex_init failure.** Initialise now checks the
return value; on failure (ENOMEM / EAGAIN under memory pressure)
installs the shared NullMutex vtable so Lock/Unlock are safe no-ops.
Cleanup gates pthread_mutex_destroy on the PosixMutex vtable being
installed, so destroy doesn't run on an uninitialised native handle.
Defensive — pthread_mutex_init essentially never fails in our
environment; the failure-path else is uncovered (no function-pointer
seam exists for testing it, and adding one for a single defensive
branch is more cost than the value).
- **PosixMessageQueueBuffer slot-indexed queue names.** Pre-fix, each
pool slot mapped to `/solidsyslog_<pid>` so bumping
`SOLIDSYSLOG_POSIX_MESSAGE_QUEUE_BUFFER_POOL_SIZE` above 1 would
alias every slot onto the same kernel queue object. Initialise gains
a `size_t slotIndex` parameter; Static.c passes the pool index;
queue name becomes `/solidsyslog_<pid>_<slotIndex>`. 64-byte name
buffer is plenty of headroom. Audience-table row in CLAUDE.md
updated.
Tested via the existing `tunable-override-debug` preset: bumped
`SOLIDSYSLOG_POSIX_MESSAGE_QUEUE_BUFFER_POOL_SIZE` to 2 in
Tests/Fixtures/SmallMessageSizeTunables.h and added a new
`EachPooledHandleHasIsolatedQueue` test (gated by
`#if POOL_SIZE >= 2`) that writes through slot 0 and confirms slot 1
doesn't see the message. Default-config build compiles the test
out; CI's build-linux-tunable-override job picks it up.
CodeRabbit's #1 (NullMutex `instance` rename to project-style) and
#4 (extract `CHECK_IS_FALLBACK` to a shared helper) deferred — the
former requires a tree-wide rename across all 11 GoF nulls (worth
its own naming-sweep story per `project_naming_sweep_deferred`); the
latter is pre-existing duplication that predates S11.06 and belongs
in a focused test-helper consolidation story.
All gates green locally: debug, sanitize, tidy, cppcheck, clang-format,
coverage. 99.5% overall line coverage; PosixMutex.c dips to 95.7% (1
defensive line uncovered).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1262 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Owner
Author
|
Re the nitpick on CodeRabbit's 3 actionable inline comments addressed in
|
Conditional compilation on a tunable hid the test from the run entirely at default pool size 1, making the result dishonest — the test simply disappeared rather than being accounted for. Replace the `#if SOLIDSYSLOG_POSIX_MESSAGE_QUEUE_BUFFER_POOL_SIZE >= 2` wrapper with a runtime guard: if the pool size can't host a second slot, UT_PRINT a notice and TEST_EXIT. The test is now in the count in every build; the default build prints "Pool size < 2 — slot isolation only observable under tunable-override-debug" and exits cleanly; the tunable-override build exercises the assertion. Switched the test body from fixture's `pooled[]` array (sized to SOLIDSYSLOG_POSIX_MESSAGE_QUEUE_BUFFER_POOL_SIZE — `pooled[1]` is out-of-bounds at pool size 1) to local `slotZero`/`slotOne` pointers. Cleanup happens before the CHECK_FALSE/LONGS_EQUAL so a failing assertion doesn't leak pool slots. Audit of the rest of the tree found no other test-side conditional compilation introduced by S11.06. Pre-existing patterns remain in Tests/SolidSyslogTunablesTest.cpp:10 (#ifdef around static_assert that proves the user-tunables override flowed through) and the standard header-guard / __cplusplus / SOLIDSYSLOG_USER_TUNABLES_FILE include mechanism / SOLIDSYSLOG_*_POOL_SIZE compile-time floor checks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1263 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
13 tasks
This was referenced May 22, 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 platform-adapter sweep in E11 (#29). Applies the canonical 3-TU split +
SolidSyslogPoolAllocatorto every stateful POSIX adapter class:PosixMutex,PosixFile,PosixTcpStream,PosixDatagram,GetAddrInfoResolver,PosixMessageQueueBuffer.Also lands four new public GoF nulls (
NullDatagram/NullStream/NullFile/NullResolver) used by this sweep and the next three (S11.07 Windows, S11.08 FreeRTOS, S11.09 TLS / FatFs / Atomics), and migratesNullMutexfrom_Create/_Destroyto the shared_Get(void)shape.Closes #405.
Change Description
Thirteen commits — four GoF-null additions, the
NullMutexshape migration, six per-class POSIX pool migrations, a local gate-sweep cleanup, and the DEVLOG entry. Each per-class migration drops the public<Class>Storagetypedef andSOLIDSYSLOG_*_SIZEenum, switches_Createto take only its real parameters (orvoidwhere there are none), and adopts the_Destroy(base)shape for the previously_Destroy(void)singletons.Three decisions worth surfacing:
GetAddrInfoResolverstays pool-allocated, not_Get. The class is truly stateless today, butFreeRtosStaticResolver(S11.08) carries IPv4 octet state, so symmetric pool semantics for the twoResolverimpls wins out over the marginal honesty of_Getfor the POSIX one alone.NullMutexshape migration is bundled in this story. It was the last GoF null still on_Create/_Destroy, and PosixMutex's pool-exhaustion fallback wiring would have updated four caller test files anyway — folding the rename in here saves a follow-up.PosixDatagram_Initialisenow explicitly setsFd = INVALID_FDandConnected = false. Pool slots are zero-initialised, andFd = 0is stdin. Closed a latent re-Create bug the singleton shape had been hiding (Fd persisted across Destroy/Create cycles).Per-class delta target unchanged from the S11.02 helper extraction: each
*Static.clands at ~3 file-scope functions (_Create,_Destroy,IndexFromHandle) + the tinyCleanupAtIndexbridge. No new MISRA findings; the three storage-cast deviations forPosixMutex/PosixFile/PosixTcpStreamevaporated.Full rationale + open questions + deferred follow-ups in the DEVLOG entry (last commit on the branch).
Test Evidence
SolidSyslogCircularBufferPool. Cover fallback-distinctness, error reporting (severity + message), ConfigLock acquisition pattern, and stale / unknown destroy reporting. Total new tests: 9 per class × 6 classes = 54, plus 1 inSolidSyslogNullMutexTestfor the_Getsingleton contract, plus 4 / 4 / 10 / 1 tests across the four new GoF nulls. +72 tests in S11.06 overall (1186 → 1258 in the POSIX suite).debug,clang-debug,sanitize(ASan + UBSan), and thecpputest-freertoscontainer (perfeedback_verify_in_freertos_host_image).SOLIDSYSLOG_USER_TUNABLES_FILEfixture.tidy,cppcheck,iwyu,clang-format.Areas Affected
Platform/Posix/Interface/— six headers lose Storage types + SIZE enums;_Create/_Destroysignatures change.Platform/Posix/Source/— three-TU split per class. Twelve new files (*Private.h+*Static.cfor each); six existing.cfiles now host_Initialise/_Cleanuprather than_Create/_Destroy.Core/Interface/— four new public GoF null headers;SolidSyslogTunablesDefaults.hgains six tunables;CLAUDE.mdaudience-table gains five rows and revises six.Core/Source/— four new GoF null.cfiles;SolidSyslogErrorMessages.hgains 12 message constants;SolidSyslogNullMutex.h/.cshape-migrated.Tests/— every PosixXxxTest gains a Pool TEST_GROUP; four new NullXxxTest files; existing teardowns updated to the_Destroy(handle)shape (POSIX callers inUdpSender,StreamSender,BlockStorePosix, BDD service-thread tests).Bdd/Targets/Linux/main.candBdd/Targets/Common/BddTargetTlsSender_OpenSsl_PosixTcp.c— dropstatic <*>Storagelocals;_Destroycalls take the handle.SolidSyslogcore (S11.10).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests