refactor: S10.17 Network primitives conformance#429
Merged
Conversation
cppcheck-misra's essential-type tracker does not follow `bool`-returning function calls across the call boundary, so `if (IsValid(...))` is flagged 14.4 even though the function genuinely returns `bool`. Verified that de-inlining the function does not help; the tracker has the same limitation against extern functions. Verified that explicit `== true` makes the controlling expression a comparison, which cppcheck-misra unambiguously recognises as essentially-Boolean. Tree-wide sweep across all 23 `*Static.c::_Create` sites that use `SolidSyslogPoolAllocator_IndexIsValid` in an if-statement. Covers the 13 sites in S10.17's network-primitives scope plus the 9 sites in `*Static.c` TUs that didn't exist when their owning per-group story ran (S10.12 buffer pilot pre-dates the E11 split for PosixMessageQueueBuffer; S10.13 pre-dates the splits for StdAtomicCounter / WindowsAtomicCounter / PosixMutex / WindowsMutex / FreeRtosMutex). Also covers MbedTlsStreamStatic (not in CI cppcheck-misra scope yet, but applying the same fix here means S10.20's scope widen verifies it pre-clean). `== true` is a code-smell shape (redundant comparison on a `bool`), but chosen over a new deviation D.012 — the explicit form makes the rule structurally satisfied at every call point with no tool-limitation exception to remember. 23 files changed, +23/-23 lines. No behaviour change.
… MISRA 8.9 Each Address Static had two file-scope statics flagged 8.9 — the pool storage array (referenced only inside `_HandleFromIndex`) and the pool-exhaustion fallback singleton (referenced only inside `_Create`). Both move inside their using function as function-scope `static`, matching S10.16's null-object 8.9 sweep. Identical lifetime, identical address, identical zero-initialisation. Scope: PosixAddressStatic.c, WinsockAddressStatic.c, FreeRtosAddressStatic.c. The fallback's explanatory comment moves with the declaration. The local names drop the `<Class>_` prefix (NAMING.md Tier 4 lowerCamelCase for locals) since the file-scope-disambiguation reason no longer applies. `PosixAddress_InUse` and `PosixAddress_Allocator` stay at file scope — `InUse` is referenced inside the Allocator initializer (file scope, not inside any function — 8.9 does not fire), and `Allocator` is referenced from both `_Create` and `_Destroy` (two functions — 8.9 does not fire). Line shifts: the move inserts ~5 lines inside `_Create` (comment + declaration) and 1 line inside `_HandleFromIndex` (declaration). Four existing D.002 11.3 suppressions on these files now have stale anchors — folded into the upcoming D.002 anchor-refresh commit.
…RA 8.9) Four `static const TickType_t` timing constants in FreeRtosTcpStream.c (`CONNECT_TIMEOUT_TICKS`, `NO_TIMEOUT_TICKS`, `ARP_RESOLUTION_WAIT_TICKS`) and FreeRtosDatagram.c (`ARP_RESOLUTION_WAIT_TICKS`) sat at file scope but each is referenced inside a single function. Move each inside its using function as a function-scope `static const` — pdMS_TO_TICKS expands to a compile-time constant given configTICK_RATE_HZ, so the initialiser remains a constant expression as required for `static` storage class. Explanatory comments move with the declarations. READ_FAILED stays at file scope — it's referenced from two functions (_Send and _Read), so 8.9 doesn't apply.
…tialise (MISRA 17.7) Three identical sites — PosixAddress, WinsockAddress, FreeRtosAddress — each calls `memset(&self->Sockaddr, 0, sizeof(self->Sockaddr))` in `_Initialise` and discards the (void*) return value implicitly. Explicit (void) cast satisfies rule 17.7.
cppcheck-misra rule 22.10 requires that errno reads sit immediately after a C-library function documented to set it, with no intervening calls. Three POSIX-idiom sites fired the rule even though the reads were correct: - PosixDatagram_SendTo: errno == EMSGSIZE in the else-if branch after `if (sent >= 0)`. Capture sendErrno into a local immediately after sendto; test the local. - PosixTcpStream_Connect: errno == EINPROGRESS in the else-if branch after `if (rc == 0)`. Capture connectErrno immediately after connect; test the local. - PosixTcpStream_WouldBlock: pure helper reading errno with no preceding errno-setting call in its own body — the caller's recv() is the source. Change signature to take `int err` as a parameter; caller (PosixTcpStream_Read) captures recvErrno immediately after recv and passes it in. The helper is now pure. No behaviour change; only the call shape is restructured to keep errno fresh against MISRA's locality requirement.
…sites + new 2.4 unmaskings Eight in-scope 5.7-on-anonymous-enum suppressions migrated from the D.003 block to the D.009 block: - FreeRtosTcpStream.c (anchor re-set :45 → :27 after Pattern C removed three file-scope timing constants and shifted the file) - TlsStream.c:16, GetAddrInfoResolver.c:20, PosixDatagram.c:21, PosixSleep.c:6, PosixTcpStream.c:24, WinsockResolver.c:37, WinsockTcpStream.c:100 Plus one new D.009 5.7 entry for FreeRtosResolver.c:21 (the GETADDRINFO_SUCCESS anonymous enum introduced by S08.08). Plus one new D.009 2.4 entry for UdpPayload.h:15 — surfaced after Pattern C narrowed the FreeRtos transport timing-constant scope. The 2.4 was always there but masked by cppcheck's dedup against the existing 5.7 suppression. Same unmasking phenomenon S10.16's DEVLOG flagged. SolidSyslogAddress.h:10 stays in D.003 (the only in-scope true struct-tag site).
…ves cluster Pattern B/C/E moves shifted line numbers in Address Statics, FreeRtos transport TUs, Posix transport TUs, and OpenSSL TlsStream. Anchor refresh on the affected D.002 11.2 / 11.3 / 11.5 suppressions, plus new D.002 entries for: - 3 new 11.2 sites at Address.c sources (Posix / Windows / FreeRtos) — cppcheck-misra now fires both 11.2 and 11.3 at the same opaque-impl downcast line, only 11.3 was historically suppressed. - 3 11.3 orphan refreshes in S10.13's territory (StdAtomicCounter, WindowsAtomicCounter, FreeRtosMutex) and 1 in S10.18's territory (FatFsFile) — pre-S08.08 anchor drift caused by the E11 *Static.c split; same fix-when-we-see-it precedent as S10.16's null-object sweep. Tree-wide cppcheck-misra in scope: 0 unsuppressed findings.
Records the per-pattern outcome across the 52-finding cluster, the de-inline experiment that didn't help, the `== true` decision over deviation D.012, the unmasked 2.4 at UdpPayload.h:15 (S10.16's dedup phenomenon revisits), and the carry-forward for S10.18 / S10.19.
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 (32)
📝 WalkthroughWalkthroughThis PR implements MISRA C:2012 conformance fixes for network primitives across POSIX, Windows, FreeRTOS, and OpenSSL adapters. Following the prescribed S10.17 workflow, it applies seven remediation patterns to resolve all unsuppressed cppcheck-misra findings while maintaining test coverage and no new violations. ChangesNetwork Primitives MISRA Conformance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 #428. Sixth per-group conformance story in E10, applying the S10.12 pilot recipe to the Network primitives cluster —
Datagram,Stream,Resolver,Address,Sleep, plus their POSIX / Windows / FreeRTOS / OpenSSL impls. Resolves all warning-mode cppcheck-misra findings in scope; clang-tidy was already clean.Change Description
CI baseline: run 26303038083 against
main@2952760(post-S08.08). 52 unsuppressed cppcheck-misra findings in scope. Seven commits, one per logical fix cluster.Pattern A — Rule 14.4 at pool-allocator IsValid call sites (tree-wide, 23 sites)
cppcheck-misra's essential-type tracker doesn't follow
bool-returning function calls across the call boundary.if (SolidSyslogPoolAllocator_IndexIsValid(...))fires 14.4 even though the function returnsbool. Two experiments before the form was chosen:IndexIsValidfrom header to.c— no effect. Same tracker limitation.if (... == true)— works. The comparison yields bool, which the tracker unambiguously sees as essentially-Boolean.== trueis a code-smell shape, but preferred over a new deviation D.012. Tree-wide sweep across all 23*Static.c::_Createsites — the 13 in S10.17's scope, the 9 orphan sites in pool TUs whose owning per-group story closed before E11 introduced the*Static.csplit (S10.12 buffer, S10.13 sync + atomics, S10.18 file), andMbedTlsStreamStatic.c(not in CI cppcheck-misra scope yet but applying the fix here means S10.20's CI scope widen verifies it pre-clean).Pattern B — Rule 8.9 Address Static pool/fallback move (3 files, 6 sites)
Each Address Static had two file-scope statics flagged 8.9 — the pool storage array (used only in
_HandleFromIndex) and the pool-exhaustion fallback singleton (used only in_Create). Both move inside their using function as function-scopestatic. Same lifetime, same address. Same pattern as S10.16's null-object 8.9 sweep.Pattern C — Rule 8.9 FreeRtos transport timing-constant move (4 sites)
FreeRtosTcpStreamandFreeRtosDatagramhad file-scopestatic const TickType_t XXX = pdMS_TO_TICKS(N)timing constants used inside a single function each. Move inside the using function.pdMS_TO_TICKSexpands to a compile-time constant givenconfigTICK_RATE_HZ.Pattern D — Rule 17.7
(void) memset(3 sites)PosixAddress,WinsockAddress,FreeRtosAddresseach callmemset(...)in_Initialiseand discard thevoid*return value implicitly. Explicit(void)cast satisfies 17.7.Pattern E — Rule 22.10 capture errno at the call site (3 sites)
Three sites: PosixDatagram
sendto, PosixTcpStreamconnect, and the sharedPosixTcpStream_WouldBlockhelper. cppcheck-misra rule 22.10 requires errno reads to sit immediately after the C-library function that may set it, with no intervening calls — theelse if (errno == ...)shape after a>= 0check trips the rule, and a helper that reads errno at the bottom of a call stack can't satisfy the locality test at all.Three fixes:
int sendErrno = (sent < 0) ? errno : 0;immediately aftersendto; test the local.connectErrno.int errso the helper is pure;_ReadcapturesrecvErrnoimmediately afterrecvand passes it in.Pattern G — D.009 widens to absorb 7 anonymous-enum 5.7 sites + 1 new entry + 1 unmasked 2.4
Seven 5.7-on-anonymous-enum suppressions migrate from the D.003 block to the D.009 block (continuing the migration started by S10.16). One new D.009 entry for
FreeRtosResolver.c:21(theGETADDRINFO_SUCCESSanonymous enum landed by S08.08). One new D.009 2.4 entry forUdpPayload.h:15— the 2.4 was always there but masked by cppcheck's dedup against the existing 5.7 suppression; surfaced once Pattern C narrowed the FreeRtos transport inclusion chain. Same unmasking phenomenon S10.16 flagged.Only one true struct-tag 5.7 in scope (
SolidSyslogAddress.h:10) stays in D.003.Pattern F — D.002 anchor refresh + new sites
Pattern B/C/E moves shifted line numbers in several files. Anchor refresh on the affected D.002 11.2 / 11.3 / 11.5 suppressions (11 in-scope refreshes), plus:
_Initialise; only 11.3 was historically suppressed.StdAtomicCounter,WindowsAtomicCounter,FreeRtosMutex,FatFsFile) — pre-existing anchor drift from the E11*Static.csplit. Same fix-when-we-see-it precedent as S10.16's null-object sweep.Test Evidence
debugbuild clean.sanitize(ASan + UBSan) clean; 1290 / 1290 tests pass.tidypreset clean (zero in-scope warnings).cppcheck-misra(with--suppressions-list=misra_suppressions.txt) — zero in-scope unsuppressed findings; verified against the sameghcr.io/davidcozens/cpputest:sha-18f19e1image the CI uses.coverage99.9% (2924 / 2928 lines, 602 / 602 functions). The four uncovered lines sit inBlockStoreStatic.candPosixMutex.c— pre-existing, no changed file lost coverage.clang-formatclean tree-wide.Areas Affected
Platform/{Atomics,FatFs,FreeRtos,MbedTls,OpenSsl,Posix,Windows}/Source/SolidSyslog*Static.c— 23 files, one-line== truechange at each_Create::IsValidcall site (Pattern A).Platform/{Posix,Windows,FreeRtos}/Source/SolidSyslog*AddressStatic.c— pool/fallback declarations moved inside using functions (Pattern B, 3 files).Platform/FreeRtos/Source/SolidSyslogFreeRtos{TcpStream,Datagram}.c— timing constants moved inside using functions (Pattern C, 2 files).Platform/{Posix,Windows,FreeRtos}/Source/SolidSyslog*Address.c— explicit(void) memset(Pattern D, 3 files).Platform/Posix/Source/SolidSyslogPosix{Datagram,TcpStream}.c— errno capture pattern (Pattern E, 2 files).misra_suppressions.txt— D.009 expansion + D.002 anchor refreshes (Patterns F, G).DEVLOG.md— entry recording per-pattern decisions.No public-API changes.
PosixTcpStream_WouldBlockis TU-private (static inline), so its signature change is invisible to integrators.Summary by CodeRabbit