feat: S08.05 store-and-forward on FatFs#352
Conversation
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGate FatFs into host/test builds when ChangesFatFs host TDD build and tests
FreeRTOS BDD target and QEMU semihosting disk
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Platform/FatFs/Source/SolidSyslogFatFsFile.c (1)
110-114: ⚖️ Poor tradeoffInterface constraint: Truncate cannot report errors.
FatFsFile_Truncatediscards return values fromf_lseek()andf_truncate(), which may fail on read-only filesystems, disk-full, or I/O errors. However, theSolidSyslogFile_Truncateinterface is defined asvoidacross all platforms (FatFs, Windows, Posix), meaning error reporting is not supported by the current contract. Addressing this would require evolving the interface signature to return an error status and updating all platform implementations and call sites.🤖 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/FatFs/Source/SolidSyslogFatFsFile.c` around lines 110 - 114, FatFsFile_Truncate currently ignores errors from f_lseek and f_truncate which violates robustness because SolidSyslogFile_Truncate is void; change the SolidSyslogFile_Truncate interface to return a status (e.g., SolidSyslogResult or int) and update FatFsFile_Truncate to propagate and return the mapped FatFs error code from f_lseek/f_truncate (check both calls and prefer the first failing error), then update the Windows and Posix implementations (and all call sites of SolidSyslogFile_Truncate) to the new return type and handle/report errors accordingly so truncation failures are no longer silently dropped.
🤖 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.
Nitpick comments:
In `@Platform/FatFs/Source/SolidSyslogFatFsFile.c`:
- Around line 110-114: FatFsFile_Truncate currently ignores errors from f_lseek
and f_truncate which violates robustness because SolidSyslogFile_Truncate is
void; change the SolidSyslogFile_Truncate interface to return a status (e.g.,
SolidSyslogResult or int) and update FatFsFile_Truncate to propagate and return
the mapped FatFs error code from f_lseek/f_truncate (check both calls and prefer
the first failing error), then update the Windows and Posix implementations (and
all call sites of SolidSyslogFile_Truncate) to the new return type and
handle/report errors accordingly so truncation failures are no longer silently
dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d67f69a-1705-4b2e-86ce-58d264856a28
📒 Files selected for processing (6)
Platform/FatFs/Interface/SolidSyslogFatFsFile.hPlatform/FatFs/Source/SolidSyslogFatFsFile.cTests/FatFs/CMakeLists.txtTests/FatFs/SolidSyslogFatFsFileTest.cppTests/Support/FatFsFakes/Interface/FatFsFake.hTests/Support/FatFsFakes/Source/FatFsFake.c
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/FatFs/CMakeLists.txt
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
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)
Bdd/Targets/FreeRtos/main.c (1)
805-871:⚠️ Potential issue | 🟠 Major | ⚡ Quick winServiceTask dereferences
g_lifecycleMutexafter Teardown nulls it.The interactive teardown sequence at Lines 826-831 destroys the lifecycle mutex and clears
g_lifecycleMutex = NULL, thenvTaskDelete(NULL). The Service task, however, keeps spinning in itsfor (;;)loop and unconditionally callsSolidSyslogMutex_Lock(g_lifecycleMutex)(Line 863). Between the momentg_lifecycleMutexbecomes NULL and QEMU being killed by the harness, the Service task will dereference NULL inside the mutex API (or block on a freed semaphore handle if FreeRTOS reuses the storage).Even on this Tier 3 BDD target this widens the failure window during
quitcleanup. Two small options:
- Have ServiceTask observe a
g_solidSyslogTeardownflag (org_lifecycleMutex == NULL) andvTaskDelete(NULL)before the lock call.- Or delete the Service task from InteractiveTask before tearing down the mutex.
🤖 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 `@Bdd/Targets/FreeRtos/main.c` around lines 805 - 871, ServiceTask can dereference g_lifecycleMutex after InteractiveTask/Teardown destroys it; fix by making ServiceTask check for teardown before calling SolidSyslogMutex_Lock: at top of the infinite loop in ServiceTask (function ServiceTask) add a guard that if g_lifecycleMutex == NULL || g_solidSyslogReady == false (or a new global flag g_solidSyslogTeardown set by Teardown) then call vTaskDelete(NULL) / break out instead of locking; ensure Teardown sets the chosen flag (or keeps setting g_lifecycleMutex = NULL) before deleting its mutex so ServiceTask will observe it and exit safely.
🧹 Nitpick comments (5)
Bdd/Targets/FreeRtos/main.c (3)
435-445: 💤 Low valueSimplify the
discard-policyliteral selection.Lines 437-440 already validated
valueis exactly one of"oldest"/"newest"/"halt", so the triplestrcmpladder on Line 443 is redundant — assigningvaluedirectly would alias anOnSet-local pointer that does not outlive the call. Stash a small lookup or just rebind by the values you already validated:♻️ Proposed simplification
- /* String literal storage — target_driver.py emits one of the three - * literals above so the pointer stays valid (no copy needed). */ - g_pendingDiscardPolicy = (strcmp(value, "newest") == 0) ? "newest" : ((strcmp(value, "halt") == 0) ? "halt" : "oldest"); + /* Bind to a process-lifetime string literal (the caller's `value` + * buffer does not outlive OnSet). */ + if (strcmp(value, "newest") == 0) { g_pendingDiscardPolicy = "newest"; } + else if (strcmp(value, "halt") == 0) { g_pendingDiscardPolicy = "halt"; } + else { g_pendingDiscardPolicy = "oldest"; } return true;🤖 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 `@Bdd/Targets/FreeRtos/main.c` around lines 435 - 445, The discard-policy branch validates value is one of "oldest"/"newest"/"halt" but then uses a ternary strcmp ladder to pick a literal; instead, avoid aliasing the caller-local value pointer and simply assign g_pendingDiscardPolicy to the corresponding static literal based on the already-performed checks (e.g. in the "discard-policy" handler: if strcmp(value,"newest")==0 set g_pendingDiscardPolicy to "newest", else if strcmp(value,"halt")==0 set to "halt", otherwise set to "oldest"); this keeps the pointer pointing to static storage and removes the redundant triple strcmp expression.
474-488: 💤 Low valueConfusing success semantics for
set store null.
return !g_currentStoreIsFile;causes a repeatset store nullto report failure once the rebuild has already flipped to file-backed. The comment says "succeed only if we're still on the default", but from the harness side (target_driver emits--storelast, deterministically), this asymmetric truth-value is hard to read and easy to misuse later. Consider returningtrueunconditionally fornull(true no-op) — the harness never asks to revert, and the rebuild ordering already guarantees--storeis the trigger.🤖 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 `@Bdd/Targets/FreeRtos/main.c` around lines 474 - 488, The current branch handling if (strcmp(name, "store") == 0) returns !g_currentStoreIsFile for the "null" value which makes a no-op sometimes report failure; change the behavior to always succeed for "null" by replacing the conditional return (!g_currentStoreIsFile) with an unconditional success (return true) in that block (leave the "file" branch calling RebuildWithFileStore() and keep g_currentStoreIsFile usage elsewhere); update the inline comment to reflect that "null" is a no-op that always reports success.
596-629: 💤 Low valueDiagnostic
printftrail inEnsureFatFsMountedlooks like the temporary patch — track removal.
[fatfs] mounting volume 0,f_mount initial -> %d,no filesystem, formatting,f_mkfs -> %d,f_mount post-mkfs -> %d,mount completemirror the diagnostic patch from the commit log. Once the QEMU@storescenarios are stable, these should fold back into the single existing[solidsyslog] fatfs mount failed: ...error path so the UART output stays scannable on success. Leaving the cleanup as a follow-up TODO is fine; flagging so it doesn't quietly become permanent.🤖 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 `@Bdd/Targets/FreeRtos/main.c` around lines 596 - 629, EnsureFatFsMounted contains temporary diagnostic printf calls that should be removed/condensed: delete the transient debug prints ("[fatfs] mounting volume 0", "[fatfs] f_mount initial -> ...", "[fatfs] no filesystem, formatting", "[fatfs] f_mkfs -> ...", "[fatfs] f_mount post-mkfs -> ...", and "[fatfs] mount complete") and keep only the canonical error path and a single concise success log if desired; leave the existing failure log "(void) printf(\"[solidsyslog] fatfs mount failed: FRESULT=%d\\n\", (int) res)" intact, ensure g_fatfsMounted is still set on success, and reference EnsureFatFsMounted, g_fatfsMounted, f_mount, f_mkfs and g_fatfs when making the change so the UART output remains scannable once QEMU `@store` is stable.Bdd/features/environment.py (1)
186-207: 💤 Low valueMove
import systo module top-level.
import sysis already implicitly available viasys.path.insertat Line 15-16 (re-importing at module level there). Re-importing insideafter_stepis harmless but unnecessary. Hoisting the import to the existingimportblock at the top keeps imports in one place.♻️ Proposed adjustment
- text = bytes(log).decode("utf-8", errors="replace") - import sys - print(f"--- last {len(log)} bytes of BDD target stdout ---", file=sys.stderr, flush=True) + text = bytes(log).decode("utf-8", errors="replace") + print(f"--- last {len(log)} bytes of BDD target stdout ---", file=sys.stderr, flush=True)🤖 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 `@Bdd/features/environment.py` around lines 186 - 207, The local import of sys inside after_step is unnecessary; move the import to the module-level import block (alongside the existing sys.path manipulation) to keep imports consolidated. Update the top of the file to add "import sys" and remove the "import sys" line from inside the after_step function (which references context.interactive_process and getattr(..., '_solidsyslog_stdout_log')). Ensure no other references are broken after removing the inner import.Bdd/Targets/FreeRtos/diskio.c (1)
66-123: ⚡ Quick winDiagnostic
printftracing in disk_initialize / DiskImageIsReady — track removal.The inline
extern int printf(...)+[diskio] ...prints on lines 68‑69, 75, 81, 87, 91, 104 match the diagnostic patch called out in the PR description ("adds tracing around f_mount/diskio operations"). They're intentional for container debugging, but please make sure they're stripped (or gated behind a debug macro) before this slice lands — otherwise everydisk_initializeon boot prints over UART, which is noisy for downstream BDD scenarios that scrape UART output for assertions.🧹 Suggested cleanup once debugging is no longer needed
DSTATUS disk_initialize(BYTE pdrv) { - extern int printf(const char* fmt, ...); - (void) printf("[diskio] disk_initialize pdrv=%u\n", (unsigned) pdrv); if (pdrv != 0) { return STA_NOINIT; } - bool ok = DiskImageIsReady(); - (void) printf("[diskio] disk_initialize ready=%u\n", (unsigned) ok); - return ok ? 0 : STA_NOINIT; + return DiskImageIsReady() ? 0 : STA_NOINIT; }🤖 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 `@Bdd/Targets/FreeRtos/diskio.c` around lines 66 - 123, Remove or guard the diagnostic semihosting printf traces in disk_initialize and DiskImageIsReady: eliminate the inline "extern int printf(...)" declarations and the "[diskio] ..." printf calls (those in disk_initialize and all prints inside DiskImageIsReady) or wrap them behind a compile-time debug macro (e.g., `#ifdef` DISKIO_DEBUG) so normal builds do not emit UART output; ensure the behavior and return logic of disk_initialize and DiskImageIsReady remain unchanged when the prints are disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Bdd/Targets/FreeRtos/CMakeLists.txt`:
- Around line 200-214: The target_include_directories for SolidSyslogBddTarget
currently adds ${FATFS_PATH}/source which can cause ff.h to pick the upstream
sample ffconf.h and create ABI mismatch; update the include list in
target_include_directories for SolidSyslogBddTarget to use ${FATFS_STAGE_DIR}
instead of ${FATFS_PATH}/source so the executable and the upstream OBJECT lib
use the same staged headers (and therefore the integrator's ffconf.h) ensuring
consistent typedefs like LBA_t/UINT across translation units.
In `@Bdd/Targets/FreeRtos/main.c`:
- Around line 561-590: RebuildWithFileStore currently ignores NULL returns from
SolidSyslogFatFsFile_Create, SolidSyslogFileBlockDevice_Create,
SolidSyslogCrc16Policy_Create, and SolidSyslogBlockStore_Create and proceeds to
call SolidSyslog_Create and set g_currentStoreIsFile/g_solidSyslogReady;
instead, after each creation call (or at least before setting
g_currentStoreIsFile and calling SolidSyslog_Create) check for NULL and on any
failure undo/cleanup any previously created resources if needed, unlock
g_lifecycleMutex, and return false so the caller/harness sees the failure;
ensure you do not flip g_currentStoreIsFile or g_solidSyslogReady nor call
SolidSyslog_Create when g_currentStore is NULL.
- Around line 631-649: The SemihostingExit function currently uses SYS_EXIT
(opcode 0x18) but passes a pointer parameter block (AArch64 style); change it to
use SYS_EXIT_EXTENDED (opcode 0x20) so the AArch32 semihosting call receives the
parameter block {reason=0x20026, status} correctly: set the register r0 to 0x20
and r1 to the address of args in SemihostingExit, keep the bkpt 0xAB and the
defensive for(;;), and update the comment to reflect SYS_EXIT_EXTENDED usage and
the parameter block format.
---
Outside diff comments:
In `@Bdd/Targets/FreeRtos/main.c`:
- Around line 805-871: ServiceTask can dereference g_lifecycleMutex after
InteractiveTask/Teardown destroys it; fix by making ServiceTask check for
teardown before calling SolidSyslogMutex_Lock: at top of the infinite loop in
ServiceTask (function ServiceTask) add a guard that if g_lifecycleMutex == NULL
|| g_solidSyslogReady == false (or a new global flag g_solidSyslogTeardown set
by Teardown) then call vTaskDelete(NULL) / break out instead of locking; ensure
Teardown sets the chosen flag (or keeps setting g_lifecycleMutex = NULL) before
deleting its mutex so ServiceTask will observe it and exit safely.
---
Nitpick comments:
In `@Bdd/features/environment.py`:
- Around line 186-207: The local import of sys inside after_step is unnecessary;
move the import to the module-level import block (alongside the existing
sys.path manipulation) to keep imports consolidated. Update the top of the file
to add "import sys" and remove the "import sys" line from inside the after_step
function (which references context.interactive_process and getattr(...,
'_solidsyslog_stdout_log')). Ensure no other references are broken after
removing the inner import.
In `@Bdd/Targets/FreeRtos/diskio.c`:
- Around line 66-123: Remove or guard the diagnostic semihosting printf traces
in disk_initialize and DiskImageIsReady: eliminate the inline "extern int
printf(...)" declarations and the "[diskio] ..." printf calls (those in
disk_initialize and all prints inside DiskImageIsReady) or wrap them behind a
compile-time debug macro (e.g., `#ifdef` DISKIO_DEBUG) so normal builds do not
emit UART output; ensure the behavior and return logic of disk_initialize and
DiskImageIsReady remain unchanged when the prints are disabled.
In `@Bdd/Targets/FreeRtos/main.c`:
- Around line 435-445: The discard-policy branch validates value is one of
"oldest"/"newest"/"halt" but then uses a ternary strcmp ladder to pick a
literal; instead, avoid aliasing the caller-local value pointer and simply
assign g_pendingDiscardPolicy to the corresponding static literal based on the
already-performed checks (e.g. in the "discard-policy" handler: if
strcmp(value,"newest")==0 set g_pendingDiscardPolicy to "newest", else if
strcmp(value,"halt")==0 set to "halt", otherwise set to "oldest"); this keeps
the pointer pointing to static storage and removes the redundant triple strcmp
expression.
- Around line 474-488: The current branch handling if (strcmp(name, "store") ==
0) returns !g_currentStoreIsFile for the "null" value which makes a no-op
sometimes report failure; change the behavior to always succeed for "null" by
replacing the conditional return (!g_currentStoreIsFile) with an unconditional
success (return true) in that block (leave the "file" branch calling
RebuildWithFileStore() and keep g_currentStoreIsFile usage elsewhere); update
the inline comment to reflect that "null" is a no-op that always reports
success.
- Around line 596-629: EnsureFatFsMounted contains temporary diagnostic printf
calls that should be removed/condensed: delete the transient debug prints
("[fatfs] mounting volume 0", "[fatfs] f_mount initial -> ...", "[fatfs] no
filesystem, formatting", "[fatfs] f_mkfs -> ...", "[fatfs] f_mount post-mkfs ->
...", and "[fatfs] mount complete") and keep only the canonical error path and a
single concise success log if desired; leave the existing failure log "(void)
printf(\"[solidsyslog] fatfs mount failed: FRESULT=%d\\n\", (int) res)" intact,
ensure g_fatfsMounted is still set on success, and reference EnsureFatFsMounted,
g_fatfsMounted, f_mount, f_mkfs and g_fatfs when making the change so the UART
output remains scannable once QEMU `@store` is stable.
🪄 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: a862d7fe-b46b-4594-bbe5-2c8c7668df5e
📒 Files selected for processing (11)
Bdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/diskio.cBdd/Targets/FreeRtos/ffconf.hBdd/Targets/FreeRtos/ffsystem.cBdd/Targets/FreeRtos/main.cBdd/features/capacity_threshold.featureBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/steps/target_driver.pyPlatform/FatFs/Interface/SolidSyslogFatFsFile.hci/docker-compose.bdd.yml
✅ Files skipped from review due to trivial changes (2)
- Bdd/features/capacity_threshold.feature
- Bdd/Targets/FreeRtos/ffconf.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Platform/FatFs/Interface/SolidSyslogFatFsFile.h
| g_storeReadFile = SolidSyslogFatFsFile_Create(&g_storeReadFileStorage); | ||
| g_storeWriteFile = SolidSyslogFatFsFile_Create(&g_storeWriteFileStorage); | ||
| g_storeBlockDevice = SolidSyslogFileBlockDevice_Create(&g_blockDeviceStorage, g_storeReadFile, g_storeWriteFile, STORE_PATH_PREFIX); | ||
|
|
||
| struct SolidSyslogSecurityPolicy* policy = SolidSyslogCrc16Policy_Create(); | ||
| struct SolidSyslogBlockStoreConfig storeConfig = { | ||
| .blockDevice = g_storeBlockDevice, | ||
| .maxBlockSize = g_pendingMaxBlockSize, | ||
| .maxBlocks = g_pendingMaxBlocks, | ||
| .discardPolicy = MapDiscardPolicy(g_pendingDiscardPolicy), | ||
| .securityPolicy = policy, | ||
| .onStoreFull = OnStoreFull, | ||
| .storeFullContext = NULL, | ||
| .getCapacityThreshold = GetCapacityThreshold, | ||
| .onThresholdCrossed = NULL, | ||
| .thresholdContext = &g_pendingCapacityThreshold, | ||
| }; | ||
| g_currentStore = SolidSyslogBlockStore_Create(&g_blockStoreStorage, &storeConfig); | ||
| g_currentStoreIsFile = true; | ||
|
|
||
| g_solidSyslogConfig.store = g_currentStore; | ||
| /* Re-honour `set no-sd 1` if it arrived before this rebuild — the | ||
| * sort order in target_driver.py guarantees `set no-sd` comes before | ||
| * `set store file` so the value is final by the time we get here. */ | ||
| g_solidSyslogConfig.sdCount = g_pendingNoSd ? 1U : (sizeof(g_sdList) / sizeof(g_sdList[0])); | ||
| SolidSyslog_Create(&g_solidSyslogConfig); | ||
| g_solidSyslogReady = true; | ||
| SolidSyslogMutex_Unlock(g_lifecycleMutex); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
RebuildWithFileStore ignores allocation failures.
SolidSyslogFatFsFile_Create, SolidSyslogFileBlockDevice_Create, SolidSyslogCrc16Policy_Create, and SolidSyslogBlockStore_Create can each return NULL (heap exhaustion / pre-mount-failure paths), but the function unconditionally proceeds to SolidSyslog_Create(&g_solidSyslogConfig) with potentially NULL .store. That tips a successful set store file reply back to the harness while the target is broken, masking the failure root cause in the diagnostic patch's printf trail.
Cheap defence: check the BlockStore result before flipping g_currentStoreIsFile = true / g_solidSyslogReady = true, return false on failure, and unlock the lifecycle mutex on the way out so the harness sees the failed set reply.
🤖 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 `@Bdd/Targets/FreeRtos/main.c` around lines 561 - 590, RebuildWithFileStore
currently ignores NULL returns from SolidSyslogFatFsFile_Create,
SolidSyslogFileBlockDevice_Create, SolidSyslogCrc16Policy_Create, and
SolidSyslogBlockStore_Create and proceeds to call SolidSyslog_Create and set
g_currentStoreIsFile/g_solidSyslogReady; instead, after each creation call (or
at least before setting g_currentStoreIsFile and calling SolidSyslog_Create)
check for NULL and on any failure undo/cleanup any previously created resources
if needed, unlock g_lifecycleMutex, and return false so the caller/harness sees
the failure; ensure you do not flip g_currentStoreIsFile or g_solidSyslogReady
nor call SolidSyslog_Create when g_currentStore is NULL.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Bdd/features/steps/target_driver.py`:
- Around line 187-194: The parsing loop currently treats the next token as a
value even if it's another flag, so in the block handling next(iterator) (where
variables are flag, value, iterator and pairs), after getting value =
next(iterator) and before appending to pairs, check whether value is a flag
token (e.g., starts with "-" or "--"); if it is, raise a ValueError similar to
the StopIteration case indicating that the key/value flag {flag!r} expects a
value but got another flag {value!r}, otherwise append (flag, value) to pairs.
Ensure the new check sits alongside the existing StopIteration handling so
malformed input like "--facility --severity" fails fast.
🪄 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: 4aea3583-d51d-42b7-8161-b2da99854456
📒 Files selected for processing (1)
Bdd/features/steps/target_driver.py
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Plumbing-only scaffolding for the FatFs adapter slices to come. No
production code yet — first SolidSyslogFatFsFile content lands at
slice 2.
Adds Platform/FatFs/ as a peer to Posix / Windows / FreeRtos / OpenSsl,
shaped as an INTERFACE library so each consumer recompiles the
(future) adapter sources with its own ffconf.h on the include path —
header-configured platform pattern, same as Platform/FreeRtos/.
Adds Tests/Support/FatFsFakes/ holding a host-suitable ffconf.h
(FF_FS_REENTRANT=0, FF_USE_LFN=0, FF_FS_NORTC=1, single 512-byte-sector
volume, FF_USE_MKFS=0) so adapter unit tests can compile against ff.h
without pulling in the real ff.c or a RAM-disk diskio. The FatFsFake.h
control surface and FatFsFake.c bodies land per-slice as adapter
behaviour drives new f_* calls.
Adds Tests/FatFs/SolidSyslogFatFsFileTest with one plumbing assertion
(FfConfRevisionMatchesFatFsHeader) that exercises the include-path
contract ff.h enforces with #error: FatFsFakes/Interface/ffconf.h is
found first, \$FATFS_PATH/source/ff.h is reachable, and the FFCONF_DEF
/ FF_DEFINED revisions agree (80386 ↔ R0.16). This test is removed at
slice 2 when the first real TEST_GROUP(SolidSyslogFatFsFile) lands.
Both Platform/FatFs and Tests/FatFs are gated on \$ENV{FATFS_PATH}
being set, matching the existing Platform/FreeRtos gating on
FREERTOS_KERNEL_PATH.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
clang-format collapses the tab-aligned columns the upstream FatFs ffconf.h uses (which mine inherited verbatim) into single-space delimited #define lines. Values unchanged — purely whitespace. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the FatFs-backed SolidSyslogFile adapter with Create/Destroy lifecycle and the Open/Close/IsOpen portion of the vtable. Read/Write/ SeekTo/Size/Truncate land in slice 3, Exists/Delete in slice 4. Public surface (Platform/FatFs/Interface/SolidSyslogFatFsFile.h) mirrors SolidSyslogPosixFile.h: SolidSyslogFatFsFileStorage opaque storage struct, SOLIDSYSLOG_FATFS_FILE_SIZE sized to hold the internal struct (FIL + base vtable + isOpen flag) on 64-bit hosts, Create takes a storage pointer and returns &storage->base. Open uses FA_READ | FA_WRITE | FA_OPEN_ALWAYS, matching PosixFile's O_RDWR | O_CREAT (read/write, create if absent). Open state is tracked explicitly via a bool field rather than reading FatFs's internal fp.obj.fs — cleaner and doesn't depend on FatFs internals. Destroy delegates to Close; Close is idempotent (guarded by isOpen), so Destroy on a never-opened or already-closed file is a safe no-op on f_close. 9 tests driven in ZOMBIES order against a new Tests/Support/FatFsFakes fake of the FatFs API surface (f_open / f_close only at this slice), not real FatFs — adapter unit tests verify the translation from SolidSyslogFile contract to FatFs calls, not FatFs's own correctness. Real FatFs only enters the build at slice 5 on QEMU. Tests use the existing CALLED_FAKE / NEVER / ONCE assertion macros from Tests/Support/TestUtils.h and a pair of CHECK_FILE_IS_OPEN / CHECK_FILE_CLOSED macros local to the test file. Static functions in the adapter follow the FatFsFile_* class-name prefix convention (memory feedback_static_name_prefix), not bare verbs like the older PosixFile precedent. Storage size is set to sizeof(intptr_t) * 90 which fits the 64-bit host TDD struct comfortably. The 32-bit FreeRTOS cross-compile sizing is revisited at slice 5 when the BDD target first instantiates storage on Cortex-M. Refs #270. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the remaining bulk-IO vtable slots to the FatFs adapter. After
this slice the SolidSyslogFile contract is fully implemented except
Exists / Delete (slice 4).
Mapping:
- Read(buf, count) -> f_read(fp, buf, count, &br);
returns true iff result == FR_OK && br == count.
- Write(buf, count) -> f_write(fp, buf, count, &bw); same shape.
- SeekTo(offset) -> f_lseek(fp, offset).
- Size() -> reads fp->obj.objsize via f_size macro.
The macro dereferences FIL state directly so the
FatFsFake captures the FIL pointer at f_open and
exposes FatFsFake_SetFileSize to populate it for
tests.
- Truncate() -> truncates to zero length:
f_lseek(fp, 0); f_truncate(fp);
FatFs's f_truncate alone would truncate at the
current fptr, which doesn't match PosixFile's
"ftruncate(fd, 0)" semantics.
9 tests added (ZOMBIES order):
- TruncateSeeksToZeroAndCallsFTruncate
- SeekToCallsFLseekWithGivenOffset (triangulation built in)
- SizeReturnsFileObjectSize
- ReadCallsFReadWithCorrectDefaults + partial + FRESULT failure
- WriteCallsFWriteWithCorrectDefaults + partial + FRESULT failure
Tests use new intent-naming CHECK_* macros:
CHECK_LSEEK_OFFSET, CHECK_READ_BUF, CHECK_READ_COUNT,
CHECK_WRITE_BUF, CHECK_WRITE_COUNT.
Production-side DRY: a Self() inline helper extracts the
(struct SolidSyslogFatFsFile*) cast that every vtable function needs;
READ_WRITE_OR_CREATE names the FA_READ | FA_WRITE | FA_OPEN_ALWAYS
flag combination passed to f_open.
FatFsFake state now grouped by FatFs function (f_open / f_close /
f_lseek / f_truncate / f_read / f_write) — each ff function has its
own state block and Reset() walks them top-down.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…12.1
FatFsFile_Read and FatFsFile_Write returned
return result == FR_OK && br == count;
MISRA C:2012 Rule 12.1 wants explicit precedence around comparisons
inside &&, so this becomes
return (result == FR_OK) && (br == count);
CLAUDE.md's "no parens needed for plain && between bool-typed operands"
covers bool *variables* / bool-returning function calls; comparison
expressions are a different case and get parens.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completes the SolidSyslogFile vtable on the FatFs adapter. Exists and
Delete are stateless with respect to the file handle — neither uses
self — so they wrap f_stat and f_unlink directly with the caller's
path.
- Exists(path) -> f_stat(path, NULL) == FR_OK
NULL FILINFO is documented as supported by FatFs
when only the existence check is needed.
- Delete(path) -> f_unlink(path) == FR_OK
6 tests added:
- ExistsCallsFStatAndReportsTrue (call shape + path + CHECK_TRUE)
- ExistsUsesPassedPath (triangulation)
- ExistsReportsFalseWhenFStatFails (FR_NO_FILE)
- DeleteCallsFUnlinkAndReportsTrue (call shape + path + CHECK_TRUE)
- DeleteUsesPassedPath (triangulation)
- DeleteReportsFalseWhenFUnlinkFails (FR_DENIED)
FatFsFake grows f_stat and f_unlink wrappers with the same shape as
the other ff functions (set-result + call-count + last-path).
After this slice, the full SolidSyslogFile contract is implemented on
FatFs. Slice 5 wires the BlockStore composition into the FreeRTOS BDD
target main.c and adds the QEMU semihosting diskio.c that makes real
FatFs run on the target.
Refs #270.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two extractions surfaced in the end-of-slice review pass:
1. Production — Fp(self) inline helper that returns &Self(self)->fp.
Five vtable functions (Read, Write, SeekTo, Size, Truncate) need
only the FIL*, not the wider SolidSyslogFatFsFile*. Replacing the
per-function
struct SolidSyslogFatFsFile* fatfs = Self(self);
... &fatfs->fp ...
pair with Fp(self) drops one local variable per function and turns
Size/SeekTo into true one-liners. Open and Close still use the
wider Self() because they also touch isOpen.
2. Tests — CHECK_OPEN_PATH, CHECK_OPEN_MODE, CHECK_STAT_PATH,
CHECK_UNLINK_PATH macros. Match the existing intent-naming pattern
(CHECK_FILE_IS_OPEN / CHECK_LSEEK_OFFSET / CHECK_READ_BUF etc.) and
replace eight bare STRCMP_EQUAL / LONGS_EQUAL invocations across
the Open / Exists / Delete tests.
No behaviour change; all 24 tests / 55 checks stay green.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires FatFs into the FreeRTOS BDD target so `set store file` over the UART tears down NullStore and rebuilds SolidSyslog with FatFsFile + FileBlockDevice + BlockStore on top of a semihosting-backed FAT image. Integrator glue under Bdd/Targets/FreeRtos/: - ffconf.h with FF_FS_REENTRANT=1, FF_USE_MKFS=1, FF_VOLUMES=1, FF_FS_NORTC=1 - ffsystem.c vendored from upstream with OS_TYPE 3 (FreeRTOS, xSemaphoreCreateMutex). FF_FS_REENTRANT is on even though only the Service task touches the store today — a future reentrancy stress test exercises the production lock path rather than a no-op - diskio.c implements disk_initialize/status/read/write/ioctl via BKPT 0xAB semihosting traps against solidsyslog-disk.img (~1 MiB sparse, 2048 x 512 B). disk_initialize creates the image on first open so f_mount returns FR_NO_FILESYSTEM and FatFs falls through to f_mkfs CMake stages ff.c / ff.h / diskio.h alongside our ffconf.h in the build dir via configure_file, so ff.h's `#include "ffconf.h"` resolves to our integrator config — works around GCC's "current file's directory first" rule without vendoring 7000+ lines of upstream FatFs into the repo. main.c lifts SolidSyslog config + state to file scope and adds a SolidSyslogMutex around the Service task vs the rebuild path. The new `set` keys max-blocks / max-block-size / discard-policy / halt-exit update pending globals; `set store file` is the rebuild trigger. Halt-exit fires SYS_EXIT (0x18) via semihosting to terminate QEMU deterministically, mirroring Linux's _exit(2). target_driver.py adds five flag translations, sorts emission so `--store` lands last, supplies `-semihosting-config enable=on, target=native` to QEMU, and synthesises a `"1"` value for bare `--halt-exit` so the UART NAME VALUE protocol stays honest. environment.py removes solidsyslog-disk.img in after_scenario. SOLIDSYSLOG_FATFS_FILE_SIZE bumped to sizeof(intptr_t) * 180 — real FatFs FIL with FF_MAX_SS=512 + FF_FS_TINY=0 is ~620 B, the previous 90-intptr constant was tuned to the smaller fake FIL. Tunable follow-up tracked for S21.03 under E21. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lights up store_and_forward / power_cycle_replay / store_capacity on the FreeRTOS BDD target. The slice-5 wiring landed BlockStore plumbing but every Store_Write was silently a no-op: FatFs requires an explicit f_mount before any file operation, and slice-5 relied on a non-existent auto-mount. With NullStore destroyed and the file-backed BlockStore unable to write, every message fell through DrainBufferIntoStore's direct-send fallback — fine while the oracle was up (first scenario message reached it) but lossy as soon as the oracle went down (the other 5 scenarios saw "1 of N" deliveries). Fix: RebuildWithFileStore now calls EnsureFatFsMounted() before tearing down the existing store. f_mount runs with opt=1 to surface FR_NO_FILESYSTEM eagerly; on a fresh disk image f_mkfs lays down a FAT12 volume and re-mounts. A mount failure leaves the target on its original NullStore (zero-disruption) and the OnSet "store" branch reports false so the harness surfaces the error rather than silently degrading. Verified locally: `set store file` now leaves a 1 MiB image with a real FAT12 boot sector (eb fe 90 MSDOS5.0, 55 aa at 510). Also lands the wiring the three @store features need: - target_driver.py: `--no-sd` joins the FreeRTOS translation table as a bare flag (synthetic "1" value), matching --halt-exit. store_capacity scenarios couple --no-sd with --store file so the sort order keeps --store last and the rebuild path picks up the final no-sd setting. - main.c: `set no-sd N` flips g_pendingNoSd; both initial Setup and RebuildWithFileStore re-honour it via sdCount = 1 (just metaSd) or 3 (full SD list). - ci/docker-compose.bdd.yml: `not @store` lifted from behave-freertos so store_and_forward, power_cycle_replay, and store_capacity all run. capacity_threshold stays out via per-feature @freertoswip until the threshold-marker plumbing has a semihosting equivalent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Locally the slice-6 f_mount path creates a valid FAT12 disk image and the first `send 1` succeeds. In David's WSL docker compose run none of that happens — no disk image is left after a scenario and every first- message step fails (0 of 1 received), which is a regression from the previous run where the first message at least reached the oracle via direct-send fallback. Need visibility into the FreeRTOS guest's UART output to find the divergence. Adds: - [fatfs] traces in main.c around f_mount / f_mkfs (entry, return codes, mount-complete confirmation). - [diskio] traces in diskio.c around disk_initialize, the semihosting open(r+b) / open(w+b) handle returns, and Flen result. - target_driver.py: route QEMU stderr to the parent process's stderr on FreeRTOS so semihosting / QEMU runtime errors surface in docker compose's behave output. - syslog_steps.py: tee the FreeRTOS guest's UART output (stdout) into a 16 KB sliding buffer maintained by the existing reader thread. - environment.py after_step hook: on step failure, dump that buffer to the behave log so we see the [fatfs] / [diskio] / SolidSyslog ErrorHandler lines that ran during the failing step. Once the docker run reveals the divergence, the diagnostic printfs and target_driver stderr passthrough get reverted; the after_step buffer dump is general enough to keep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…no() The slice-6 diagnostic patch routed QEMU stderr to sys.stderr on the FreeRTOS target so semihosting errors would surface in docker compose output. behave's runner wraps sys.stderr with an internal stream that doesn't support fileno(), so subprocess.Popen raised io.UnsupportedOperation on every spawn — breaking all FreeRTOS scenarios, not just the failing @store ones. Revert to stderr=subprocess.PIPE everywhere. The sliding-buffer + after_step dump (added in the same diagnostic chore) still gives us visibility into [fatfs] / [diskio] / [solidsyslog] guest UART output on step failure, which is the actual diagnostic we want. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundles the major CodeRabbit finding fix, three minor cleanups, and the focused FatFs self-test David suggested to isolate the docker BDD regression. ServiceTask race on teardown (CodeRabbit major, also the slice-5 smoke-test hard-fault root cause): InteractiveTask's quit-time teardown destroyed g_lifecycleMutex and NULLed the pointer; ServiceTask's next iteration unconditionally locked it, dereferencing NULL or use-after- free freed kernel state — `qemu: fatal: Lockup` on every quit. Fix: add a g_solidSyslogTeardown flag, set inside the lifecycle-mutex critical section so Service observes it atomically with the SolidSyslog destroy. Service unlocks, vTaskDelete(NULL) before any further mutex access. Teardown then vTaskDelay(20ms) — generous against Service's worst-case iteration — before destroying the mutex. Local smoke test: `quit` now prints the stack-hwm report and exits cleanly with no HardFault escalation. Three minor CodeRabbit findings: - main.c: `set store null` returns true unconditionally now (was !g_currentStoreIsFile, asymmetric truth value). The harness always emits --store last so the rebuild ordering is the real guard. - environment.py: hoist `import sys` to the module top-level block (was repeated inside after_step). - target_driver.py: apply_extra_args fails fast when a key/value flag is followed by another flag instead of a value — `--facility --severity 6` would have silently treated `--severity` as facility's value. FatFs self-test (David's suggestion in lieu of broader instrumentation): runs the exact sequence BlockStore + FileBlockDevice will use — open(W), write, close, stat-exists, open(R), read, close, unlink, stat-not-exists — immediately after EnsureFatFsMounted reports success. Each step prints [fatfs-test] with its FRESULT. Locally all nine steps return 0 / FR_NO_FILE=4 as expected. If docker shows any non-zero, we have the exact failing operation. If docker shows the same all-pass, the FatFs layer is sound and the regression lives above it (BlockStore / Sender / Service). Diagnostic printfs from the prior chore commit stay in place pending the docker investigation; cleanup is tracked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The OBJECT lib (containing ff.c + our ffsystem.c) compiles against
FATFS_STAGE_DIR, where our integrator ffconf.h is colocated with the
staged ff.h. But the executable target (main.c, diskio.c,
SolidSyslogFatFsFile.c) pointed at ${FATFS_PATH}/source. When those
.c files include "ff.h" from that path, ff.h's `#include "ffconf.h"`
resolves to upstream's sample in the same directory before searching
-I paths — so the executable's translation units saw FF_FS_REENTRANT=0
(upstream default) while ff.c saw FF_FS_REENTRANT=1 (ours).
Any ffconf.h-conditional layout (FF_FS_REENTRANT controls the sync
object, FF_USE_LFN controls lfnbuf, FF_MAX_SS vs FF_MIN_SS controls
ssize, etc.) would then differ between translation units, opening the
door to silent FATFS struct corruption. Locally the specific combo
happens not to trip an observable failure, but the divergence is wrong
on its face and is a plausible candidate for the docker BDD regression
(where the disk image is created and FAT is laid down correctly but
oracle receives 0 messages).
Fix: point the executable at FATFS_STAGE_DIR too, with a comment
explaining why it must mirror the OBJECT lib's include path. Local
smoke test re-verified: build clean, every FatFs self-test step
passes (return 0 / FR_NO_FILE=4 as expected), `quit` exits without
HardFault.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… path Suspected docker BDD regression root cause. FileBlockDevice (Core/Source/SolidSyslogFileBlockDevice.c) caches readHandle and writeHandle independently, each holding an underlying FIL open between calls. When the store has a single active block (the common case during a short outage burst), BOTH handles target the same path: writeHandle has STORE00.LOG open from the first Append, readHandle opens STORE00.LOG for the first SendOneFromStore read. With FF_FS_LOCK=0 in ffconf.h, the FatFs manual explicitly warns: "0: Disable file lock function. To avoid volume corruption, application program should avoid illegal open, remove and rename to the open objects." Two concurrent FILs on one path is "illegal open" — undefined behaviour, can silently corrupt the volume's FAT chain. On POSIX (the Linux BDD target) two fds on one path are fine; the FileBlockDevice contract was designed against that, so the same dual-handle pattern silently breaks on FatFs+FF_FS_LOCK=0 while passing on Linux. Setting FF_FS_LOCK to a positive value enables FatFs's internal open- file tracking, which permits multiple FILs on one path with correct shared-write semantics. 4 gives headroom over the 2 FileBlockDevice holds (in case rename/delete opens a third FIL transiently inside f_unlink). Memory cost: ~12 B per slot = 48 B static, trivial against our 96 KB heap. Local smoke test (single-FIL self-test) unchanged — sequential ops weren't hitting the lock path anyway. Validation comes from the BDD @store scenarios which exercise FileBlockDevice's concurrent pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
S08.05 slice 6 is paused pending a store-layer refactor that removes the "two file handles open on the same path" requirement. See memory entry project_s08_05_slice6_status for the full architectural finding and the next-session brief. This commit snapshots the slice-6-diagnostic state so the branch is clean across context switches: - Bdd/Targets/FreeRtos/ffconf.h: clang-format fix that was queued for the next slice-6-targeted commit. Standalone hygiene; not load-bearing. - Bdd/Targets/FreeRtos/main.c: RunFatFsDualFilProbe — a boot-time probe that walks the FileBlockDevice dual-handle open pattern and prints FRESULT traces. Designed to surface FatFs's lock semantics (chk_share at ff.c:970 rejects 2nd write-mode open with FR_LOCKED) without BDD scaffolding. Keep as evidence; revert when slice 6 resumes and greens. - Tests/Support/FatFsFakes/Source/FatFsFake.c: f_sync stub returning FR_OK so FatFsFake-linked unit tests stay green if a future iteration reintroduces f_sync calls. Harmless future-proofing. When slice 6 resumes (post-refactor on main), revert this commit or strip the probe + earlier [fatfs]/[diskio]/[fatfs-test] diagnostic prints together as a cleanup commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yslogFile* per S27.01 The S27.01 refactor (PR #354, on main) dropped FileBlockDevice's readHandle/writeHandle pair in favour of a single handle. The Linux and Windows BDD targets were updated at that time; the FreeRTOS target lived on this branch and missed the cascade. This commit cascades: - Replaces g_storeReadFile / g_storeReadFileStorage / g_storeWriteFile / g_storeWriteFileStorage with a single g_storeFile / g_storeFileStorage. - Updates SolidSyslogFileBlockDevice_Create and matching Destroys. - Drops RunFatFsDualFilProbe and its caller — the probe walked FileBlockDevice's dual-FIL pattern to surface the FatFs FF_FS_LOCK rejection. With S27.01 the pattern is gone, the rejection can't happen, and the probe has nothing left to prove. Cross-built clean against the freertos-cross preset. FF_FS_LOCK=4 walk-back, [fatfs]/[diskio]/[fatfs-test] diagnostic-print strip, and the FatFsFake f_sync stub are separate housekeeping commits to follow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…the same path" This reverts commit f72cc1a.
The [fatfs] / [diskio] / [fatfs-test] traces and RunFatFsSelfTest were instrumentation added during the S08.05 slice 6 pause to surface the dual-FIL hazard root-caused in PR #354 (S27.01). With the refactor merged and the FF_FS_LOCK=4 workaround walked back, the FatFs layer's behaviour is no longer the suspect — the next iteration of slice-6 debugging is at the store-and-forward drain level, where this noise just masks the interesting bits. The [solidsyslog] mount-failed error line stays — that's a genuine failure path the integrator wants to see. Cross-built clean against the freertos-cross preset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Tests/Support/FatFsFakes/Source/FatFsFake.c`:
- Around line 203-212: The fake f_read reports the wrong number of bytes when
readBytesReturnedOverridden is false: it always sets *br = btr even if only
copyCount (min(btr, readSourceCount)) bytes were copied. Change the assignment
so that when readBytesReturnedOverridden is true you keep *br =
readBytesReturned, otherwise set *br = copyCount (where copyCount is the UINT
computed as (btr <= readSourceCount) ? btr : readSourceCount) so the caller sees
the actual number of bytes copied into buff; update references in f_read
(readBytesReturnedOverridden, readBytesReturned, btr, copyCount,
readSourceCount) accordingly.
🪄 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: 82767d8d-8a6f-4804-b5f0-d1f33fbf5035
📒 Files selected for processing (27)
.gitignoreBdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/diskio.cBdd/Targets/FreeRtos/ffconf.hBdd/Targets/FreeRtos/ffsystem.cBdd/Targets/FreeRtos/main.cBdd/features/capacity_threshold.featureBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/steps/target_driver.pyCMakeLists.txtPlatform/FatFs/CMakeLists.txtPlatform/FatFs/Interface/SolidSyslogFatFsFile.hPlatform/FatFs/Source/SolidSyslogFatFsFile.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.cTests/CMakeLists.txtTests/FatFs/CMakeLists.txtTests/FatFs/SolidSyslogFatFsFileTest.cppTests/FatFs/main.cppTests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cppTests/Support/FatFsFakes/CMakeLists.txtTests/Support/FatFsFakes/Interface/FatFsFake.hTests/Support/FatFsFakes/Interface/ffconf.hTests/Support/FatFsFakes/Source/FatFsFake.cTests/Support/FreeRtosFakes/Interface/FreeRtosSocketsFake.hTests/Support/FreeRtosFakes/Source/FreeRtosSocketsFake.cci/docker-compose.bdd.yml
✅ Files skipped from review due to trivial changes (2)
- Tests/Support/FatFsFakes/CMakeLists.txt
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (17)
- CMakeLists.txt
- Bdd/features/capacity_threshold.feature
- Bdd/Targets/FreeRtos/ffconf.h
- Tests/FatFs/main.cpp
- Platform/FatFs/CMakeLists.txt
- Platform/FatFs/Interface/SolidSyslogFatFsFile.h
- Bdd/features/environment.py
- Bdd/Targets/FreeRtos/CMakeLists.txt
- Tests/CMakeLists.txt
- Bdd/Targets/FreeRtos/ffsystem.c
- Tests/FatFs/SolidSyslogFatFsFileTest.cpp
- Bdd/features/steps/target_driver.py
- Platform/FatFs/Source/SolidSyslogFatFsFile.c
- Tests/FatFs/CMakeLists.txt
- Bdd/Targets/FreeRtos/diskio.c
- Bdd/Targets/FreeRtos/main.c
- Tests/Support/FatFsFakes/Interface/ffconf.h
When SetReadSource programs fewer bytes than the caller requests, the fake now sets *br = copyCount (bytes actually memcpy'd into buff) rather than *br = btr (bytes requested). Production's `(result == FR_OK) && (br == count)` partial-read check now sees the short read instead of treating uninitialised buffer memory as valid data. CodeRabbit catch on PR #352. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
after_scenario already removes solidsyslog-disk.img on the happy path, but a Ctrl-C / OOM / debugger-detach during a prior run leaves the image behind. diskio.c::DiskImageIsReady keeps any existing full-size image, so a stale FAT with STORE*.log content carries silently into the next scenario's mount. Idempotent FileNotFoundError-tolerant remove in before_scenario is the belt to after_scenario's brace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLAUDE.md bans Hungarian notation and member-variable prefixes; the FreeRTOS BDD-target main.c had crept past the rule (~30 file-scope identifiers). Pure rename, contained to Bdd/Targets/FreeRtos/main.c — no other file references the old names. testMessage takes the special-case name to avoid shadowing ErrorHandler's `message` parameter; everything else just drops g_. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, the `quit` command and `set shutdown 1` had two separate exit paths. `quit` ran the full destroy chain (SolidSyslog + SDs + counter + store + Buffer + Mutex + Senders + Streams + Datagram + Resolver). `set shutdown 1` only ran a partial chain (SolidSyslog + store + f_unmount), leaving the Senders / Streams / Datagram / Resolver dangling — most importantly the TCP stream's open connection to syslog-ng-freertos, which on SemihostingExit leaves the peer with an ungracefully closed connection. Single source of truth now: TeardownAll() does the full chain plus f_unmount. Two callers: - `quit`: BddTargetInteractive_Run returns → TeardownAll → vTaskDelete - `set shutdown 1`: OnSet → TeardownAll → SemihostingExit(0) The Setup-locals (resolver, datagram, tcpStream, tcpSender, buffer, bufferMutex) become file-scope static so both entry points reach them. No g_* prefix — convention CLAUDE.md mandates. The standalone ShutdownGracefully is gone; DestroyCurrentStore remains as a small helper shared by RebuildWithFileStore and TeardownAll. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1114 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/Support/FatFsFakes/Source/FatFsFake.c (1)
130-165: ⚡ Quick winMake
f_close,f_lseek, andf_truncateresults injectable for error-path tests.These three hooks always return
FR_OK, while other hooks are configurable. This limits coverage of adapter failure handling for close/seek/truncate paths.Suggested diff
/* f_close state */ static int closeCallCount; +static FRESULT closeResult; /* f_lseek state */ static int lseekCallCount; static FSIZE_t lastLseekOffset; +static FRESULT lseekResult; /* f_truncate state */ static int truncateCallCount; +static FRESULT truncateResult; void FatFsFake_Reset(void) { @@ closeCallCount = 0; + closeResult = FR_OK; lseekCallCount = 0; lastLseekOffset = 0; + lseekResult = FR_OK; truncateCallCount = 0; + truncateResult = FR_OK; @@ } + +void FatFsFake_SetCloseResult(FRESULT result) +{ + closeResult = result; +} @@ FRESULT f_close(FIL* fp) { (void) fp; closeCallCount++; - return FR_OK; + return closeResult; } + +void FatFsFake_SetLseekResult(FRESULT result) +{ + lseekResult = result; +} @@ FRESULT f_lseek(FIL* fp, FSIZE_t ofs) { (void) fp; lseekCallCount++; lastLseekOffset = ofs; - return FR_OK; + return lseekResult; } + +void FatFsFake_SetTruncateResult(FRESULT result) +{ + truncateResult = result; +} @@ FRESULT f_truncate(FIL* fp) { (void) fp; truncateCallCount++; - return FR_OK; + return truncateResult; }🤖 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/Support/FatFsFakes/Source/FatFsFake.c` around lines 130 - 165, The functions f_close, f_lseek and f_truncate always return FR_OK which prevents testing error paths; change them to return injectable result variables (e.g., closeResult, lseekResult, truncateResult) instead of the hardcoded FR_OK, update f_lseek to still set lastLseekOffset and increment lseekCallCount but return lseekResult, and add simple setter functions (e.g., FatFsFake_SetCloseResult, FatFsFake_SetLseekResult, FatFsFake_SetTruncateResult) so tests can configure failure values; reference the existing symbols closeCallCount, lseekCallCount, truncateCallCount and lastLseekOffset when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Bdd/features/steps/target_driver.py`:
- Around line 197-201: The current guard in target_driver.py that raises
ValueError when value.startswith("-") (inside the extra_args/FreeRTOS parsing
where variables flag and value are checked) wrongly rejects legitimate
hyphen-prefixed values; change the predicate so it only treats value as “another
flag” when it matches a known flag token instead of any hyphen prefix—e.g.,
introduce or reuse a set/list of known FreeRTOS flags (e.g.
FREE_RTOS_KNOWN_FLAGS or known_flags) and replace the condition with a
membership check (or normalize to the flag name when handling assignments like
--foo=bar) so only values present in that known-flag set trigger the ValueError
while allowing other hyphen-prefixed values like "-hello".
---
Nitpick comments:
In `@Tests/Support/FatFsFakes/Source/FatFsFake.c`:
- Around line 130-165: The functions f_close, f_lseek and f_truncate always
return FR_OK which prevents testing error paths; change them to return
injectable result variables (e.g., closeResult, lseekResult, truncateResult)
instead of the hardcoded FR_OK, update f_lseek to still set lastLseekOffset and
increment lseekCallCount but return lseekResult, and add simple setter functions
(e.g., FatFsFake_SetCloseResult, FatFsFake_SetLseekResult,
FatFsFake_SetTruncateResult) so tests can configure failure values; reference
the existing symbols closeCallCount, lseekCallCount, truncateCallCount and
lastLseekOffset when making these changes.
🪄 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: fa36df16-84e1-44c9-9592-6b5d3f42e397
📒 Files selected for processing (27)
.gitignoreBdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/diskio.cBdd/Targets/FreeRtos/ffconf.hBdd/Targets/FreeRtos/ffsystem.cBdd/Targets/FreeRtos/main.cBdd/features/capacity_threshold.featureBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/steps/target_driver.pyCMakeLists.txtPlatform/FatFs/CMakeLists.txtPlatform/FatFs/Interface/SolidSyslogFatFsFile.hPlatform/FatFs/Source/SolidSyslogFatFsFile.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.cTests/CMakeLists.txtTests/FatFs/CMakeLists.txtTests/FatFs/SolidSyslogFatFsFileTest.cppTests/FatFs/main.cppTests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cppTests/Support/FatFsFakes/CMakeLists.txtTests/Support/FatFsFakes/Interface/FatFsFake.hTests/Support/FatFsFakes/Interface/ffconf.hTests/Support/FatFsFakes/Source/FatFsFake.cTests/Support/FreeRtosFakes/Interface/FreeRtosSocketsFake.hTests/Support/FreeRtosFakes/Source/FreeRtosSocketsFake.cci/docker-compose.bdd.yml
✅ Files skipped from review due to trivial changes (4)
- Bdd/features/capacity_threshold.feature
- .gitignore
- Tests/Support/FatFsFakes/CMakeLists.txt
- Tests/Support/FatFsFakes/Interface/ffconf.h
🚧 Files skipped from review as they are similar to previous changes (15)
- Tests/FatFs/main.cpp
- Platform/FatFs/CMakeLists.txt
- Platform/FatFs/Interface/SolidSyslogFatFsFile.h
- Tests/CMakeLists.txt
- Bdd/Targets/FreeRtos/ffconf.h
- ci/docker-compose.bdd.yml
- CMakeLists.txt
- Tests/Support/FatFsFakes/Interface/FatFsFake.h
- Tests/FatFs/CMakeLists.txt
- Bdd/Targets/FreeRtos/ffsystem.c
- Bdd/Targets/FreeRtos/CMakeLists.txt
- Platform/FatFs/Source/SolidSyslogFatFsFile.c
- Tests/FatFs/SolidSyslogFatFsFileTest.cpp
- Bdd/Targets/FreeRtos/diskio.c
- Bdd/Targets/FreeRtos/main.c
Mirrors the SolidSyslogFreeRtosDatagram ARP-prime that landed in S08.03 slice 3b.1.5. Before slice 6's SO_RCVTIMEO fix, FreeRTOS_connect defaulted to 50 s of patience and that absorbed the cold-start ARP resolution delay invisibly; with the connect now bounded at 200 ms, a cold first SYN gets dropped at the IP layer while ARP resolves and the bounded timer expires before the retransmit-SYN cycle completes. PrimeArpIfMissing runs before every connect: xIsIPInARPCache, on miss fire FreeRTOS_OutputARPRequest then vTaskDelay 50 ms so the reply has landed by the time SYN goes out. Symmetric with the Datagram path. Hypothesis: this also explains why tcp_reconnect:7 and tcp_transport:9 regressed in CI between slice 6 baseline (8a23ac6) and slice 7's first push (b217a38) — both single-shot cold-connect scenarios with no Service retry to absorb the first dropped SYN. CI will confirm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The next-token-is-another-flag guard rejected any value starting with '-', which would refuse legitimate hyphen-prefixed values like `--message -hello`. Match against _FREERTOS_SET_TRANSLATION instead so only actual known flag tokens trigger the ValueError. CodeRabbit catch on PR #352. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CodeRabbit nit on Production code ( |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1114 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
On AArch32 ARM Semihosting, SYS_EXIT (0x18) treats R1 as a *literal*
reason code, not a parameter-block pointer. Passing &{reason, status}
as R1 resolved to "unrecognised reason" → QEMU exit 1 regardless of
the status field. SYS_EXIT_EXTENDED (0x20) is the form that accepts
a {reason, subcode} parameter block and propagates subcode as the
exit status.
Caught by store_capacity.feature:37 — Halt stops asserts exit code 2
but kept seeing 1. CodeRabbit flagged this on the prior round as a
🔴 follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1114 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Host-side integration test over the real BlockStore + BlockSequence + RecordStore + FileBlockDevice stack with FileFake at the bottom. Motivated by the discard-newest BDD failure on freertos-cross — oracle saw [1, 11, 2, 3, 4, 5, 6] which on its face looks like a BlockStore drain ordering bug. The first reproducer test in this harness (Outage drain produces ascending sequenceIds) PASSES — so BlockStore alone isn't the culprit. That moves the search up one layer to the SolidSyslog_Service drain algorithm (Buffer -> Store -> Sender) which is where the [1, 11, 2..] interleave actually arises. Follow-up commits will add a Service-level test and the fix. The harness drives the Store interface directly (Write / HasUnsent / ReadNextUnsent / MarkSent), parameterised by maxBlocks / maxBlockSize / payloadSize / discardPolicy via a DrainTestConfig struct so future tests can sweep configurations without rewriting the fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires BufferFake-replacement (real CircularBuffer + NullMutex) + real BlockStore + a local SenderSpy with sticky outage mode to drive SolidSyslog_Service through the exact BDD discard-newest scenario shape at unit-test speed. When TEST'd (not IGNORE_TEST'd), the assertion fires at: Send order descended: ids[2]=2 after ids[1]=11 Reproducing the BDD failure host-side: oracle log [1, 11, 2, 3] — sequenceId 11 (newest, queued in CircularBuffer at the moment of oracle recovery) bypasses the older stored 2, 3, ... via DrainBufferIntoStore's fallback-to-Sender_Send path (Core/Source/SolidSyslog.c:269-272). That path is correct for NullStore configurations but breaks the discard-newest promise on BlockStore: "newest discarded" turns into "newest delivered first." Marked IGNORE_TEST so CI stays green until the next commit lands the fix (Store_IsTransient vtable method gating the fallthrough). The next commit flips this to TEST and the assertion goes from RED to GREEN. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Store_IsTransient to the Store vtable, distinguishing stores that never retain (NullStore, NilStore) from those that retain durably (BlockStore). Service's DrainBufferIntoStore now gates its fallthrough- to-Sender_Send on transience: only transient stores can bypass to the sender on Write rejection. This fixes the BDD discard-newest failure on freertos-cross. The previous Service algorithm fell through to direct-send whenever Store_Write returned false. On BlockStore in discard-newest mode this meant: when the store filled and rejected the *newest* buffered message, the message escaped via direct send the instant the oracle recovered — bypassing the older retained records that had been queued during the outage. Oracle saw [1, 11, 2, 3, 4, 5, 6] when it should have seen [1, 2, 3, 4, 5, 6] (with 11 discarded by the discard-newest policy). The host-side reproducer test in the previous commit (RED, IGNORE_TEST) is now flipped to TEST and goes green: successful-send order is [1, 2, 3] — no bypass. Vtable additions: - NullStore: IsTransient = true (matches its "never retain" contract). - NilStore (internal fallback before Create / on NULL config.store): IsTransient = true (same role). - BlockStore: IsTransient = false (rejection is the discard policy speaking; messages do not escape). - StoreFake (test): IsTransient = false (models a real store). The existing test ServiceSendsDirectlyWhenStoreWriteFails pinned the old fallthrough-on-StoreFake-rejection behaviour. Renamed and updated to ServiceDoesNotBypassToSenderWhenNonTransientStoreRejectsWrite, asserting the new contract. NullStore-side fallthrough is still covered by ServiceSendsBufferedMessageWithNullStore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1116 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
- Add [[nodiscard]] + const-qualify WriteMessage / DrainOne / DrainAll;
the test methods are pure observers of fixture state.
- NOLINTNEXTLINE for the SenderSpy reinterpret_cast (vtable downcast
pattern), the swappable {sequenceId, payloadSize} pair (distinct
concepts, both numeric is incidental), and the snprintf calls used
to build CppUTest FAIL messages.
- Drop the diagnostic printfs that were useful during bug investigation
but added vararg-call surface for tidy without buying anything for
the persisted test.
- Derive payloadSize from SOLIDSYSLOG_MAX_MESSAGE_SIZE so the Service-
level reproducer overflows the store on both default (MAX=2048) and
tunable-override (MAX=512) builds — the test pins drain ordering, not
a fixed byte count.
- Trim <string.h>, add <cstdio> for snprintf — IWYU hygiene.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1116 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Bdd/features/steps/syslog_steps.py (1)
1124-1149: ⚡ Quick winGraceful shutdown logic is sound, but consider hardening the kill fallback.
The FreeRTOS shutdown sequence correctly attempts graceful termination via
set shutdown 1before falling back tokill(). However, the secondprocess.wait(timeout=5)afterprocess.kill()at line 1143 is not wrapped in exception handling. WhileSIGKILLshould terminate immediately, an uncaughtTimeoutExpiredhere would propagate and potentially leavecontext.interactive_processalive.🛡️ Optional hardening for the kill fallback
except subprocess.TimeoutExpired: process.kill() - process.wait(timeout=5) + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + # Kill should terminate immediately; if we still timeout, log and proceed + pass🤖 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 `@Bdd/features/steps/syslog_steps.py` around lines 1124 - 1149, The second process.wait(timeout=5) after calling process.kill() (operating on context.interactive_process) can raise subprocess.TimeoutExpired and is not caught; wrap that wait in a try/except for subprocess.TimeoutExpired and handle it (e.g., call process.kill() again or process.kill() + process.terminate() fallback, optionally log the failure) to ensure the process is cleaned up and the subsequent del context.interactive_process always runs; update the FreeRTOS branch around the process.kill() / process.wait sequence to catch TimeoutExpired and proceed safely.Core/Source/SolidSyslogStore.c (1)
42-45: ⚡ Quick winAdd a conservative fallback when
IsTransientis not wired.Line 44 assumes every integrator-provided store vtable already sets
IsTransient. A legacy/custom store missing this field can crash on the first write rejection path. Defaulting tofalsekeeps ordering-safe behavior.Proposed patch
bool SolidSyslogStore_IsTransient(struct SolidSyslogStore* store) { - return store->IsTransient(store); + if (store->IsTransient == NULL) + { + return false; + } + return store->IsTransient(store); }As per coding guidelines,
Core/Interface/**/*.h: Tier 1 public headers are a stable API boundary with the highest review bar.🤖 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/SolidSyslogStore.c` around lines 42 - 45, SolidSyslogStore_IsTransient currently blindly calls the integrator-provided vtable entry and can crash if the IsTransient function pointer is NULL; update SolidSyslogStore_IsTransient to check the store->IsTransient pointer on the SolidSyslogStore struct and return false as a conservative fallback when it is NULL, otherwise invoke the function pointer normally to preserve current behavior.Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp (1)
323-332: ⚡ Quick winReplace duplicated manual ordering checks with a shared
CHECK_*macro.The same assertion shape is hand-implemented twice using
if (...) FAIL(...). Use a macro once and reuse in both tests to reduce duplication and failure-message drift. As per coding guidelines, Test code: use CHECK_* macros for repeated assertion shapes; implement as macros (not functions) with // NOLINTBEGIN wrapper and do { ... } while(0) for single-statement safety.Also applies to: 367-376
🤖 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/SolidSyslogBlockStoreDrainOrderingTest.cpp` around lines 323 - 332, Replace the duplicated manual ordering checks (the for-loop that uses if (...) FAIL(...) with snprintf) by defining a reusable test macro (e.g., CHECK_ASCENDING_ORDER(ids, count)) implemented as a macro (not a function) wrapped with // NOLINTBEGIN/END and structured as do { ... } while(0) for single-statement safety; the macro should iterate over the container (using the same logic that checks ids[i] < ids[i-1]) and call FAIL(...) with the same formatted message via snprintf when an out-of-order pair is found, then replace both occurrences (the loop around ids and the duplicate at the later location) with calls to the new macro to remove duplication and keep failure messages consistent.
🤖 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 `@Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp`:
- Around line 109-185: Extract the duplicated storage/test fixture into a shared
TEST_GROUP_BASE (e.g., BlockStoreTestBase) that contains the members
fileStorage, file, deviceStorage, device, storeStorage, store, policy and the
setup()/teardown() implementations plus helper methods CreateStore(),
WriteMessage(), DrainOne(), and DrainAll(); then have
TEST_GROUP(BlockStoreDrainOrdering) (and the other TEST_GROUP at 198-280)
inherit from that base and remove the duplicated members and lifecycle code from
their bodies so both tests reuse the single centralized fixture.
- Around line 153-160: The WriteMessage helper writes sequence bytes into
buf[0..3] without validating payloadSize and there are corresponding decoding
sites that read payload[0..3] unchecked; add guards so both the writer and all
places that decode the sequence (the WriteMessage function and the
payload-decoding code paths in this test file that access payload[0..3]) first
verify payload size >= 4 (e.g., if payloadSize < 4 return false / fail the test
or skip parsing), and handle the error path deterministically to avoid OOB
access and flaky crashes.
- Around line 273-279: ServiceTickUntilQuiet can under-drain because it always
runs a fixed number of SolidSyslog_Service() ticks; change it to poll until the
syslog reports no more work and return a success flag so the test can assert
recovery completed. Specifically, modify ServiceTickUntilQuiet(size_t cap) to
call SolidSyslog_Service() in a loop up to cap but break early when a readiness
check (e.g., SolidSyslog_IsIdle() or SolidSyslog_HasPending()—use the
appropriate existing status function) indicates no pending work, return true if
idle reached and false on cap exhaustion, and update the test call to
ServiceTickUntilQuiet(10) to assert the returned value is true (or otherwise
verify progress/completion) instead of relying only on monotonic order.
---
Nitpick comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 1124-1149: The second process.wait(timeout=5) after calling
process.kill() (operating on context.interactive_process) can raise
subprocess.TimeoutExpired and is not caught; wrap that wait in a try/except for
subprocess.TimeoutExpired and handle it (e.g., call process.kill() again or
process.kill() + process.terminate() fallback, optionally log the failure) to
ensure the process is cleaned up and the subsequent del
context.interactive_process always runs; update the FreeRTOS branch around the
process.kill() / process.wait sequence to catch TimeoutExpired and proceed
safely.
In `@Core/Source/SolidSyslogStore.c`:
- Around line 42-45: SolidSyslogStore_IsTransient currently blindly calls the
integrator-provided vtable entry and can crash if the IsTransient function
pointer is NULL; update SolidSyslogStore_IsTransient to check the
store->IsTransient pointer on the SolidSyslogStore struct and return false as a
conservative fallback when it is NULL, otherwise invoke the function pointer
normally to preserve current behavior.
In `@Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp`:
- Around line 323-332: Replace the duplicated manual ordering checks (the
for-loop that uses if (...) FAIL(...) with snprintf) by defining a reusable test
macro (e.g., CHECK_ASCENDING_ORDER(ids, count)) implemented as a macro (not a
function) wrapped with // NOLINTBEGIN/END and structured as do { ... } while(0)
for single-statement safety; the macro should iterate over the container (using
the same logic that checks ids[i] < ids[i-1]) and call FAIL(...) with the same
formatted message via snprintf when an out-of-order pair is found, then replace
both occurrences (the loop around ids and the duplicate at the later location)
with calls to the new macro to remove duplication and keep failure messages
consistent.
🪄 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: 3ad2d860-fecd-4163-ae93-1d4d8023c841
📒 Files selected for processing (37)
.gitignoreBdd/Targets/FreeRtos/CMakeLists.txtBdd/Targets/FreeRtos/diskio.cBdd/Targets/FreeRtos/ffconf.hBdd/Targets/FreeRtos/ffsystem.cBdd/Targets/FreeRtos/main.cBdd/features/capacity_threshold.featureBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/steps/target_driver.pyCMakeLists.txtCore/Interface/SolidSyslogStore.hCore/Interface/SolidSyslogStoreDefinition.hCore/Source/SolidSyslog.cCore/Source/SolidSyslogBlockStore.cCore/Source/SolidSyslogNullStore.cCore/Source/SolidSyslogStore.cPlatform/FatFs/CMakeLists.txtPlatform/FatFs/Interface/SolidSyslogFatFsFile.hPlatform/FatFs/Source/SolidSyslogFatFsFile.cPlatform/FreeRtos/Source/SolidSyslogFreeRtosTcpStream.cTests/CMakeLists.txtTests/FatFs/CMakeLists.txtTests/FatFs/SolidSyslogFatFsFileTest.cppTests/FatFs/main.cppTests/FreeRtos/CMakeLists.txtTests/FreeRtos/SolidSyslogFreeRtosTcpStreamTest.cppTests/SolidSyslogBlockStoreDrainOrderingTest.cppTests/SolidSyslogTest.cppTests/StoreFake.cTests/Support/FatFsFakes/CMakeLists.txtTests/Support/FatFsFakes/Interface/FatFsFake.hTests/Support/FatFsFakes/Interface/ffconf.hTests/Support/FatFsFakes/Source/FatFsFake.cTests/Support/FreeRtosFakes/Interface/FreeRtosSocketsFake.hTests/Support/FreeRtosFakes/Source/FreeRtosSocketsFake.cci/docker-compose.bdd.yml
✅ Files skipped from review due to trivial changes (4)
- Tests/Support/FreeRtosFakes/Interface/FreeRtosSocketsFake.h
- .gitignore
- Tests/Support/FatFsFakes/CMakeLists.txt
- Platform/FatFs/Interface/SolidSyslogFatFsFile.h
🚧 Files skipped from review as they are similar to previous changes (13)
- CMakeLists.txt
- Tests/FatFs/main.cpp
- Tests/FatFs/CMakeLists.txt
- Bdd/features/capacity_threshold.feature
- Platform/FatFs/CMakeLists.txt
- Tests/Support/FatFsFakes/Interface/ffconf.h
- Tests/CMakeLists.txt
- Bdd/Targets/FreeRtos/CMakeLists.txt
- ci/docker-compose.bdd.yml
- Bdd/Targets/FreeRtos/ffconf.h
- Bdd/Targets/FreeRtos/diskio.c
- Bdd/Targets/FreeRtos/ffsystem.c
- Bdd/Targets/FreeRtos/main.c
| [[nodiscard]] bool WriteMessage(uint32_t sequenceId, size_t payloadSize) const | ||
| { | ||
| std::vector<uint8_t> buf(payloadSize, 'x'); | ||
| buf[0] = static_cast<uint8_t>(sequenceId & 0xFFU); | ||
| buf[1] = static_cast<uint8_t>((sequenceId >> 8) & 0xFFU); | ||
| buf[2] = static_cast<uint8_t>((sequenceId >> 16) & 0xFFU); | ||
| buf[3] = static_cast<uint8_t>((sequenceId >> 24) & 0xFFU); | ||
| return SolidSyslogStore_Write(store, buf.data(), buf.size()); |
There was a problem hiding this comment.
Guard 4-byte sequence encoding/decoding preconditions.
Line 153 and Line 263 write buf[0..3], and Line 81 / Line 173 decode payload[0..3] without checking size. A future smaller payload/read will cause OOB access and flaky crashes.
Proposed fix
[[nodiscard]] bool WriteMessage(uint32_t sequenceId, size_t payloadSize) const
{
+ CHECK_TRUE_TEXT(payloadSize >= 4U, "payloadSize must be >= 4 to encode sequence id");
std::vector<uint8_t> buf(payloadSize, 'x');
@@
static uint32_t DecodeSequenceId(const std::vector<uint8_t>& payload)
{
+ CHECK_TRUE_TEXT(payload.size() >= 4U, "payload must be >= 4 bytes");
return static_cast<uint32_t>(payload[0]) | (static_cast<uint32_t>(payload[1]) << 8) | (static_cast<uint32_t>(payload[2]) << 16) |
(static_cast<uint32_t>(payload[3]) << 24);
}
@@
[[nodiscard]] uint32_t DrainOne() const
{
@@
CHECK_TRUE(SolidSyslogStore_ReadNextUnsent(store, buf, sizeof(buf), &bytesRead));
+ CHECK_TRUE_TEXT(bytesRead >= 4U, "record must be >= 4 bytes to decode sequence id");
SolidSyslogStore_MarkSent(store);
@@
void Enqueue(uint32_t sequenceId, size_t payloadSize) const
{
+ CHECK_TRUE_TEXT(payloadSize >= 4U, "payloadSize must be >= 4 to encode sequence id");
std::vector<uint8_t> buf(payloadSize, 'x');Also applies to: 263-270, 81-85, 169-174
🤖 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/SolidSyslogBlockStoreDrainOrderingTest.cpp` around lines 153 - 160, The
WriteMessage helper writes sequence bytes into buf[0..3] without validating
payloadSize and there are corresponding decoding sites that read payload[0..3]
unchecked; add guards so both the writer and all places that decode the sequence
(the WriteMessage function and the payload-decoding code paths in this test file
that access payload[0..3]) first verify payload size >= 4 (e.g., if payloadSize
< 4 return false / fail the test or skip parsing), and handle the error path
deterministically to avoid OOB access and flaky crashes.
There was a problem hiding this comment.
Declining. payloadSize in this fixture is derived from either SOLIDSYSLOG_MAX_MESSAGE_SIZE - 100 (>= 412 on the smallest tunable-override build) or hardcoded 64 — the < 4 scenario can't reach the writer. CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." The guards add noise without buying real safety. If a future contributor genuinely shrinks the payload below 4 the existing buf[0..3] will assert/segfault on first run and surface the misconfiguration loudly.
There was a problem hiding this comment.
@DavidCozens, understood — the constraint is fully explained by the derivation from SOLIDSYSLOG_MAX_MESSAGE_SIZE - 100 / hardcoded 64, and the CLAUDE.md guideline is clear. I'll defer to the project philosophy here.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 352
File: Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp:175-182
Timestamp: 2026-05-13T22:05:22.272Z
Learning: In the `solid-syslog` project, the CLAUDE.md guideline explicitly discourages adding defensive error handling or validation for scenarios that cannot happen given the existing constraints. For example, test fixture helpers like `WriteMessage` in `Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp` intentionally omit payload-size guards because `payloadSize` is always derived from `SOLIDSYSLOG_MAX_MESSAGE_SIZE - 100` (>= 412) or hardcoded values (e.g., 64), making the < 4 case unreachable. Do not suggest adding such guards in this codebase.
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.
IWYU: drop unused SolidSyslogFile.h include from the test file — the forward declaration that arrives via SolidSyslogFileBlockDevice.h is enough since the test only holds the struct as an opaque pointer. CodeRabbit accepts: - Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp: extract a shared DrainTestFixtureBase (TEST_BASE) holding the file / block-device / null-security-policy fixture; both TEST_GROUPs derive from it, eliminating ~14 lines of duplicated setup/teardown plumbing. Matches the existing BlockDeviceTestBase pattern in SolidSyslogBlockStoreTest.cpp. - Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp:: DiscardNewestDoesNotLetNewestBypassOldestOnRecovery: assert !HasUnsent + successfulSends > 1 after the recovery drain window — catches under-drain (ServiceTickUntilQuiet's fixed cap exhausting before the store is empty) instead of silently passing on monotonic order alone. - Bdd/features/steps/syslog_steps.py::step_client_is_killed: wrap the post-SIGKILL process.wait in try/except so a degenerate runtime that blocks even on SIGKILL doesn't leak context.interactive_process. CodeRabbit declines (replied on threads): - SolidSyslogStore_IsTransient NULL guard — CLAUDE.md "trust internal code; don't add fallbacks for scenarios that can't happen." None of the other Store_* wrappers guard their vtable methods; adding it for IsTransient alone would mask a wiring bug we want to surface loudly. - 4-byte payload precondition CHECK_TRUE_TEXT guards — payloadSize is derived from SOLIDSYSLOG_MAX_MESSAGE_SIZE (>= 512) or hardcoded 64; the < 4 scenario can't reach the writer. Guards only add noise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai responses to the two review-body nitpicks:
The |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1116 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…ked docs CLAUDE.md: add SolidSyslogFatFsFile.h row under Public header audiences (ChaN FatFs adapter, f_sync per write, integrator-supplied diskio.c). DEVLOG: full S08.05 entry — design decisions (FatFs over Plus-FAT, Platform/FatFs/ peer pack, f_sync-per-write, graceful shutdown for BDD power-cycle, unified TeardownAll, ARP-prime on TCP connect, SYS_EXIT_EXTENDED for AArch32 status propagation, Store_IsTransient to keep discard-newest honest), the host-side BlockStore drain- ordering harness that pinned the [1, 11, 2, 3, ...] bug, ChaN license-compatibility note (BSD-style ⊂ PolyForm Noncommercial 1.0.0 — preserve ChaN's notice in ff.c), deferred items. README: FreeRTOS support paragraph now reflects UDP+TCP and FatFs store-and-forward; add SolidSyslogFreeRtosTcpStream + SolidSyslogFatFsFile to the platform-helpers list; FreeRTOS BDD target row mentions the new `set store file` / `set shutdown 1` UART commands and the store_and_forward / power_cycle_replay / store_capacity scenarios that now run on QEMU. Bdd/Targets/FreeRtos/README.md: dedicated section on persistent store-and-forward via FatFs over semihosting — covers the BDD diskio.c shape, the production-integrator handoff (own diskio.c + ffsystem.c), and the graceful shutdown contract. Bdd/README.md + docs/bdd.md: FreeRTOS local-run and per-runner tag filters caught up to what `ci/docker-compose.bdd.yml` actually uses (`not @wip and not @freertoswip and not @rtc and not @windows_wip and (@udp or @tcp)`). docs/containers.md: cpputest-freertos-cross row now mentions the FreeRTOS-Kernel / Plus-TCP / FatFs source mounts that the cross image carries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1116 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Implements story #270 — store-and-forward on FatFs for the FreeRTOS QEMU
BDD target, so messages survive an oracle outage and a power cycle by
routing through
SolidSyslogFileBlockDevice/SolidSyslogBlockStoreagainst a new
SolidSyslogFatFsFileadapter.Slicing per the issue body (8 slices, one commit each; PR raised after
slice 1 and force-pushed as slices land):
SolidSyslogFatFsFileCreate + Open/Close/IsOpendiskio.c+ FreeRTOSffsystem.c+ BDD target BlockStore wiringstore_and_forward.featuregreen on QEMUpower_cycle_replay.featuregreen on QEMUstore_capacity.featureall 4 scenarios green on QEMUCloses #270.
Change Description
Slice 1 (this commit). Plumbing-only. No production code or fakes
yet — both land in slice 2.
Platform/FatFs/added as a peer toPlatform/Posix/Windows/FreeRtos/OpenSsl. Shaped as anINTERFACElibrary so eachconsumer recompiles the adapter against its own
ffconf.h—header-configured platform pattern (same as
Platform/FreeRtos/),rationale documented inline in
Platform/FatFs/CMakeLists.txt.Tests/Support/FatFsFakes/Interface/ffconf.hprovides ahost-suitable FatFs config (single 512-byte-sector volume,
FF_FS_REENTRANT=0,FF_USE_LFN=0,FF_FS_NORTC=1,FF_USE_MKFS=0) so adapter unit tests compile againstff.hwithout the real
ff.cor a RAM-diskdiskio.c. The fake itselfis empty at slice 1 and grows per-slice as the adapter's
f_*surface drives new behaviour.
Tests/FatFs/SolidSyslogFatFsFileTestplaceholder exe with oneplumbing assertion that exercises the contract
ff.henforces with#error: this TU findsFatFsFakes/Interface/ffconf.hfirst onthe include path, reaches
$FATFS_PATH/source/ff.h, and theFFCONF_DEF/FF_DEFINEDrevisions agree (80386↔ R0.16).The test is removed at slice 2 when the first real
TEST_GROUP(SolidSyslogFatFsFile)lands.Platform/FatFsandTests/FatFsare gated on$ENV{FATFS_PATH}being set, matchingPlatform/FreeRtosgatingon
FREERTOS_KERNEL_PATH.Design note: we fake the FatFs API, not run real FatFs. Initial
plan proposed a RAM-disk
diskio.cso unit tests could run actualFatFs against an in-memory backing buffer. Course-corrected: that tests
FatFs's own correctness, not the adapter's. Slices 2–4 fake
f_open/f_read/f_write/f_lseek/f_truncate/f_stat/f_unlinkin
Tests/Support/FatFsFakes/Source/FatFsFake.c, matching the existingSocketFake/WinsockFake/OpenSslFake/FreeRtosSocketsFakepattern. Real FatFs only enters the build at slice 5 for the QEMU BDD
target, via a semihosting-backed
diskio.cunderBdd/Targets/FreeRtos/.Test Evidence
Slice 1 adds one plumbing test:
TEST(FatFsPlumbing, FfConfRevisionMatchesFatFsHeader)—LONGS_EQUAL(FF_DEFINED, FFCONF_DEF). Compiles only ifff.handffconf.hresolve correctly; runs only if the placeholder exe links.Locally on
freertos-host(BUILD_PRESET=debug):Existing test suites all stay green (full junit run completes).
Areas Affected
Platform/FatFs/(peer to Posix/Windows/FreeRtos/OpenSsl)Tests/Support/FatFsFakes/,Tests/FatFs/CMakeLists.txt,Tests/CMakeLists.txt(FATFS_PATH-gated subdir wiring + junit registration)
Slices 2–4 will add
Platform/FatFs/{Interface,Source}/SolidSyslogFatFsFile.{h,c}and grow the FatFsFake. Slice 5 will add
Bdd/Targets/FreeRtos/{SemihostingDiskIo,FfSystemFreeRtos,ffconf}.{c,h}and wire BlockStore into
Bdd/Targets/FreeRtos/main.c.Refs #270.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores