refactor: S10.18 Storage stack conformance#431
Merged
Conversation
`SolidSyslogPoolAllocator_FreeIfInUse` returns `bool` (false when the slot wasn't ours / already free). The four call sites in pool-backed `*Static.c` Destroy and Create-rollback paths legitimately don't care about that return — the operation is best-effort cleanup. cppcheck-misra 17.7 flags the implicit discard. Per-site fix: explicit `(void)` cast. Four sites: - BlockSequenceStatic.c:37 — Destroy - RecordStoreStatic.c:36 — Destroy - SolidSyslogBlockStoreStatic.c:55 — Create rollback on BlockSequence failure - SolidSyslogBlockStoreStatic.c:60 — Create rollback on RecordStore nil-object
Five 17.7 sites in RecordStore.c flagged as discarded function-call
returns. Treated per-site:
- Lines 127, 332: `(void) memcpy(...)` — copy into a byte buffer from
an opaque source (`const void* data` or `void* dst`); cppcheck-misra
is happy with the explicit-discard cast.
- Line 477: `(void) SolidSyslogBlockDevice_Read(...)` —
`_IsRecordSent` deliberately falls through to `flag == SENT_FLAG_SENT`
on a read failure during the corruption-recovery scan. The function
doc-comment (lines 462–467) already documents the fail-safe: a
skipped record surfaces downstream as a sequenceId gap.
- Lines 126, 250: the bare `(void) memcpy` cast didn't satisfy MISRA —
it unmasked rule 21.15 ("pointer arguments shall be to compatible
essential types") because the writes/reads were `uint8_t*` ↔
`uint16_t*`. Refactored to explicit little-endian byte pack/unpack:
lengthBytes[0] = (uint8_t) (length & 0xFFU);
lengthBytes[1] = (uint8_t) ((length >> 8) & 0xFFU);
Every supported target is little-endian (POSIX x86/x64/ARM, Windows
x86/x64, FreeRTOS-Cortex-M3 on QEMU); the existing
`SolidSyslogBlockStoreTest.cpp:1555` truncated-header fixture already
bakes in LE layout (`{0xA5, 0x5A, 0x05}` — length 5 packed as
`0x05 0x00`). Pure refactor on every host; the explicit shape locks
the on-disk format invariant in code rather than leaving it implicit.
All 1290 tests green on `debug`.
cppcheck-misra fires Rule 8.9 on `Core/Source/SolidSyslogFileBlockDevice.c:20` (`FILE_EXTENSION`), claiming the constant's identifier appears in only a single function. The claim is wrong — `FILE_EXTENSION` is referenced by both the file-scope enum at line 25 (`sizeof(FILE_EXTENSION) - 1U` → `FILENAME_SUFFIX` → `MAX_PREFIX_LENGTH`) **and** by `_FormatBlockFilename` at line 214. The tracker counts only function-scope references, so the enum site is invisible to it. Experimentally verified during S10.18 that promoting the dependent enum entries to file-scope `static const size_t` does not satisfy the rule — instead it surfaces a second 8.9 false positive on the new constant for the same reason. The fix path amplifies the problem rather than resolving it. New deviation row D.012 in `docs/misra-deviations.md`; single-site line-anchored suppression in `misra_suppressions.txt`. The story body specified "no new deviation rows in `docs/misra-deviations.md`" — D.012 is a divergence raised as work per the per-group conformance workflow's "raise as work" clause, since the per-site review surfaced a genuine cppcheck-misra tracker false positive rather than a fixable code defect. Divergence documented in the DEVLOG entry.
… → D.009 Per-site review of every D.003 / Rule 5.7 entry in the storage stack scope confirms all 8 are anonymous-enum sites used as named-constant containers — not the genuine struct-tag repetition pattern D.003 originally covered. Migrated to the D.009 block: - Core/Source/BlockSequence.c:13 (MIN/MAX_MAX_BLOCKS, SEQUENCE_MODULUS) - Core/Source/BlockSequence.c:107 (MAX_SEQUENCE) - Core/Source/RecordStore.c:14 (MAGIC/LENGTH/SENT_FLAG sizes + bytes) - Core/Source/RecordStorePrivate.h:14 (RECORD_BUFFER_SIZE) - Core/Source/SolidSyslogFileBlockDevice.c:16 (MAX_PATH_SIZE) - Core/Source/SolidSyslogFileBlockDevice.c:23 (SEQUENCE_DIGITS, MAX_BLOCK_INDEX, etc.) - Platform/Posix/Source/SolidSyslogPosixFile.c:18 (INVALID_FD) - Platform/Windows/Source/SolidSyslogWindowsFile.c:24 (INVALID_FD, ACCESS_EXISTENCE_CHECK) No corresponding 2.4 entries surfaced after the migration — the cppcheck dedup pattern that unmasked UdpPayload.h:15 in S10.17 doesn't fire here (no inclusion-chain narrowing in this change). Side observation captured during the per-site review: typedef-ing these anonymous enums to give the type a name would eliminate the 5.7+2.4 declarations site but immediately trip rule 10.4 at every arithmetic use site (the enum's essential type stops composing with plain integers). Net result: trades 2-per-declaration for N-per-use-site, where N is typically larger. The D.009 deviation implicitly documents this trade — the anonymous-enum-as-constant- container pattern is the cheaper shape per MISRA. Tree-wide 5.7 totals stay at 41 (21 in D.003 = genuine struct-tag sites + 20 in D.009 = anonymous-enum sites; previously 29 + 12). Out-of-scope D.003 5.7 entries kept where they are: CircularBuffer.c (closed S10.12), Crc16.c / Crc16Policy.c (closed S10.13), and the SolidSyslog.c + various platform impls deferred to S10.19. Fix when their owning groups are next touched.
clang-format pushes the `(void) FreeIfInUse(...)` lines added by Pattern A past the line-length limit and splits them across `(void` / `)` on consecutive lines — ugly but what the formatter chooses. Accepting the formatter's output rather than adding helper scaffolding to dodge it. Also picks up the column-alignment shift in RecordStore.c's AssembleRecord block after Pattern B removed two of the member-style aligned assignments.
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 (7)
📝 WalkthroughWalkthroughThis PR resolves ten cppcheck-misra findings in the storage stack (BlockSequence, RecordStore, BlockStore, BlockDevice, FileBlockDevice) by applying explicit void-casts to discarded return values, adjusting RecordStore length serialization to explicit little-endian packing, documenting a new MISRA deviation for a false-positive cppcheck 8.9 finding, and reorganizing suppression entries per S10.18 scope. ChangesStorage Stack MISRA C:2012 Conformance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1296 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Closes #430. Seventh per-group conformance story in E10 — Storage stack
cluster (
Store,BlockStore,RecordStore,BlockSequence,BlockDevice,FileBlockDevice,File, plus POSIX / Windows / FatFsfile impls). Resolves the 10 unsuppressed cppcheck-misra findings in
scope baselined at run 26309037474 (
main@a062d39, post-S10.17).Change Description
Six commits, one per logical fix cluster (Patterns A–F as enumerated in
the story body), plus a clang-format follow-up and the DEVLOG.
Pattern A —
(void) PoolAllocator_FreeIfInUse(...)at four*Static.cDestroy / Create-rollback sites. The pool API'sboolreturn is best-effort; the discards are intentional and now explicit.
Pattern B — five 17.7 sites in
RecordStore.c. Three are straight(void) memcpy(...)discards; one is(void) BlockDevice_Readon thecorruption-recovery scan path where the read-failure fall-through is
the documented fail-safe. The remaining two memcpys on the on-disk
length field were rewritten as explicit little-endian byte pack/unpack
because the bare
(void) memcpyunmasked rule 21.15 (incompatibleuint8_t*/uint16_t*essential types). Pure refactor on every host(all little-endian); existing
SolidSyslogBlockStoreTest.cpp:1555fixture already bakes in LE layout. The explicit pack locks the on-disk
format invariant.
Pattern C — new deviation
D.012
authorising the 8.9 false positive on
Core/Source/SolidSyslogFileBlockDevice.c:20::FILE_EXTENSION.cppcheck-misra's 8.9 tracker counts only function-scope references and
misses the file-scope enum's
sizeof(FILE_EXTENSION)use; the rulefires spuriously, and three restructure alternatives were rejected for
DRY / amplification-of-problem / inconsistency reasons (detailed in the
deviation row and DEVLOG). Divergence from the story's "no new
deviation rows" acceptance, raised as work per the per-group
conformance workflow's "raise as work" clause.
Patterns D and F — pure-review patterns: all 12 existing in-scope
suppressions (5 × D.002, 4 × D.004, 2 × D.006, 1 × D.008) reviewed
per-site; every rationale holds, no anchor drift after Patterns A–C.
No code change.
Pattern E — eight anonymous-enum 5.7 sites migrated from D.003 to
D.009. All sites confirmed as anonymous-enum-as-named-constant-container
by per-site reading; the D.003 block now contains only genuine
struct-tag repetition sites in scope. No 2.4 entries unmasked by the
migration. Side observation captured in DEVLOG: typedef-ing the
anonymous enums to give them a name eliminates 5.7+2.4 on the
declaration but immediately surfaces 10.4 at every arithmetic use site —
net-worse trade.
Test Evidence
All in-scope local gates green:
analyze-tidy— 0 warnings on storage-stack files (already clean on main; no regression).analyze-cppcheck(with misra addon) — 0 in-scope unsuppressed findings (down from 10); 0 new findings tree-wide.analyze-format— clean tree-wide within CI scope.debug— 1290/1290 tests pass.sanitize— 1290/1290 tests pass (ASan + UBSan).coverage— 99.9% (2925/2929 lines, 602/602 functions); +1 line covered vs. main as a side effect of Pattern B's byte-pack rewrite.Areas Affected
Core/Source/RecordStore.c(Pattern B —(void)casts and on-disk length byte pack/unpack; behaviour-preserving on all supported little-endian targets).Core/Source/BlockSequenceStatic.c,Core/Source/RecordStoreStatic.c,Core/Source/SolidSyslogBlockStoreStatic.c(Pattern A —(void)casts).docs/misra-deviations.md— new D.012.misra_suppressions.txt— added D.012 single-site suppression; migrated 8 anonymous-enum 5.7 sites D.003 → D.009.DEVLOG.md— session entry.Summary by CodeRabbit
Refactor
Documentation