feat: S12.07 PosixMessageQueueBuffer error path completeness#443
Conversation
PMQB was the only Platform/Posix adapter unit-testing against real
Linux POSIX message queues — violated the "fake the platform API
for adapter unit tests" convention (S08.05 FatFsFake precedent;
existing SocketFake/WinsockFake/OpenSslFake siblings).
Add Tests/Support/MqFake.{h,c} exporting mq_open / mq_send /
mq_receive / mq_close / mq_unlink with an in-memory queue registry
and one-shot failure-injection knobs mirroring SocketFake.
Migrate the 18 existing PMQB tests to call MqFake_Reset() in
setup(). Behaviour-preserving — no new error codes yet. Add
MqFakeTest.cpp self-tests pinning the fake's contracts.
Foundation for S12.07 error-path slices. Real mq_* continues to
exercise PMQB end-to-end at the BDD layer.
PMQB Create previously stored the mq_open result without checking it. On failure (e.g. invalid maxMessages, kernel rlimit), Create returned a broken handle whose subsequent mq_send / mq_receive silently failed with EBADF, and the acquired pool slot leaked. Thread the mq_open result back from Initialise. On failure, Create releases the slot (so the pool stays accurate for the next caller) and returns the shared SolidSyslogNullBuffer fallback, emitting the new POSIXMESSAGEQUEUEBUFFER_ERROR_MQ_OPEN_FAILED error. Two tests pin the behaviour: error emission and slot release. Slice 1 of #118.
PMQB Write previously discarded the mq_send return value, so any failure (queue full, oversized message, EBADF on a broken handle) was silent — the record was dropped with no visibility to the caller's error handler. Check the mq_send result and emit POSIXMESSAGEQUEUEBUFFER_ERROR_SEND_FAILED on failure. Write's contract stays fire-and-forget (the record is still dropped on the floor); honesty surfaces via the error handler rather than a return code, matching StreamSender. Test pins the EAGAIN (queue full) path. EMSGSIZE (oversized) goes through the same uniform check and lands in slice 3 as a regression guard. Slice 2 of #118.
Regression guard. The production check in PosixMessageQueueBuffer_Write is errno-agnostic — `if (mq_send(...) != 0)` catches EAGAIN, EMSGSIZE, EBADF, etc. uniformly — so this test passes without a production change. A future contributor who adds errno-specific handling has to deliberately drop one of these cases. Slice 3 of #118.
PMQB Read previously folded every mq_receive failure into a silent `return false`. EAGAIN (empty queue) is part of the polling happy path and rightly stays silent, but other errnos (EMSGSIZE on a short caller buffer, EBADF on a broken handle, EINTR, etc.) deserve to surface to the error handler. Classify the mq_receive errno: EAGAIN stays silent; any other failure emits the new POSIXMESSAGEQUEUEBUFFER_ERROR_RECEIVE_FAILED. Read's return contract is unchanged — still false on any failure. Two tests: EMSGSIZE-via-MqFake pins the new error emit; empty-queue poll pins that EAGAIN stays silent (regression guard). Slice 4 of #118.
PosixMessageQueueBuffer_Read previously dereferenced bytesRead unconditionally (`*bytesRead = ...`), so passing NULL would segfault. Early-return false if bytesRead is NULL. No new error code — this is invalid caller usage, not a runtime failure. The Read contract still says "false on failure, bytesRead holds the count on success". Slice 5 of #118.
Both behaviours are already safe by construction: - After Destroy, the abstract-base vtable is overwritten with the shared SolidSyslogNullBuffer's, so stale-handle Write/Read is a no-op rather than a NULL-fn-pointer crash. Mirrors the equivalent PassthroughBuffer test. - Destroy(NULL) routes via IndexFromHandle → POOL_SIZE → IndexIsValid false → UNKNOWN_DESTROY warning. The existing DestroyOfUnknownHandleReportsWarning test exercises a stack-allocated stranger; the new test pins literal NULL specifically. Both pass first time — regression guards for an integrator who strays into either case. Slice 6 of #118.
Every test case enumerated in the PMQB IGNORE_TEST block is now either pinned by a real test (slices 1–6) or explicitly out of scope (NULL-buffer dispatcher guards belong to SolidSyslog_Log, not PMQB; the blocking-mode pointer at "S4.5 or later" was stale and deserves its own story if still wanted). Also re-flow a Messages.c line that clang-format coalesced onto a single line after RECEIVE_FAILED landed in slice 4. Slice 7 of #118 — closes the story.
End-of-story IWYU sweep caught a transitive include — MqFakeTest references ssize_t (return type of mq_receive) but only got it through <mqueue.h>'s implementation. Add the direct include. Slice 7 follow-up of #118.
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 30 minutes and 32 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements comprehensive error handling for POSIX message queue buffers by adding three new error codes, changing ChangesPOSIX Message Queue Buffer Error Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 (✔️ 1324 passed, 🙈 1 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/Support/MqFake.c (1)
226-253: ⚡ Quick winReorder helpers to match this repo’s C file top-down rule.
MqFake_AllocateQueue/MqFake_QueueFromMqdare helper functions but are defined before their first caller and without top-of-file forward declarations. Move helpers beneath first caller and keep forward declarations at file top.As per coding guidelines: “Within source files, order functions top-down… Helper functions must be forward-declared at file top and defined immediately beneath their first caller for top-down reading”.
🤖 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/MqFake.c` around lines 226 - 253, Move the helper function definitions MqFake_AllocateQueue and MqFake_QueueFromMqd so they appear immediately beneath their first caller (not before it), and add forward declarations for both at the top of the C file; specifically, add prototypes for MqFake_AllocateQueue(void) and MqFake_QueueFromMqd(mqd_t) near the other top-of-file declarations, then cut the two full definitions and paste them right after the first function that calls them so the file follows the repo’s top-down ordering rule.
🤖 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/MqFake.c`:
- Around line 357-364: mq_send in MqFake currently uses the global
MQFAKE_MAX_MESSAGES_PER_QUEUE and silently truncates oversize messages; change
it to enforce each queue's configured limits: in mq_send use queue->maxMessages
to detect a full queue and set errno = EAGAIN and return -1, and before copying
check if msgLen > queue->maxMessageSize and if so set errno = EMSGSIZE and
return -1 (do not truncate); adjust the copy logic (storeLen) to be used only
after msgLen validated against queue->maxMessageSize.
---
Nitpick comments:
In `@Tests/Support/MqFake.c`:
- Around line 226-253: Move the helper function definitions MqFake_AllocateQueue
and MqFake_QueueFromMqd so they appear immediately beneath their first caller
(not before it), and add forward declarations for both at the top of the C file;
specifically, add prototypes for MqFake_AllocateQueue(void) and
MqFake_QueueFromMqd(mqd_t) near the other top-of-file declarations, then cut the
two full definitions and paste them right after the first function that calls
them so the file follows the repo’s top-down ordering rule.
🪄 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: ecb745e1-ae94-4c42-9c93-a91ab2b454a7
📒 Files selected for processing (11)
Platform/Posix/Interface/SolidSyslogPosixMessageQueueBufferErrors.hPlatform/Posix/Source/SolidSyslogPosixMessageQueueBuffer.cPlatform/Posix/Source/SolidSyslogPosixMessageQueueBufferMessages.cPlatform/Posix/Source/SolidSyslogPosixMessageQueueBufferPrivate.hPlatform/Posix/Source/SolidSyslogPosixMessageQueueBufferStatic.cTests/CMakeLists.txtTests/MqFakeTest.cppTests/SolidSyslogPosixMessageQueueBufferTest.cppTests/Support/CMakeLists.txtTests/Support/MqFake.cTests/Support/MqFake.h
CI's analyze-cppcheck job runs a second MISRA-addon invocation that exits non-zero on unsuppressed violations; my local sweep only covered the plain cppcheck invocation. Four violations slipped through: 1. PMQB.c errno test after mq_receive triggered Rule 22.10 — the predicate's value could conceivably depend on errno being set by something other than the immediately preceding library call. Captured errno into a local right after mq_receive, matching the project's existing pattern in PosixTcpStream.c:190 and PosixDatagram.c:101. No suppression needed. 2-4. Three pre-existing line-pinned suppressions (misra-c2012-11.3 PMQB.c, 5.7 PMQB.c, 5.7 PMQBPrivate.h) shifted line numbers because slices 1-5 added includes and grew the functions. Updated the pinned line numbers in misra_suppressions.txt — no new suppressions added. Local cppcheck-misra now exits 0 across all 147 production files.
CodeRabbit caught that MqFake's mq_send ignored the per-queue attributes captured at mq_open: - Queue-full path used the fake-wide MQFAKE_MAX_MESSAGES_PER_QUEUE (8) instead of queue->maxMessages, so a queue opened with maxMessages=2 would accept 8 sends before EAGAIN — diverging from the real kernel. - Oversized messages were silently truncated via storeLen clamping rather than failing with -1/EMSGSIZE per POSIX. Both gaps would mask real-kernel behaviour from tests that exercise natural overflow rather than MqFake_FailNext* injection. Per the "fakes must model the API faithfully" principle from feedback_fake_platform_apis (the precedent that drove introducing MqFake in S12.07 slice 0), fix while the fake is fresh. Enforcement order matches POSIX (mq_send specification): 1. EBADF — bad descriptor 2. EMSGSIZE — msgLen > mq_attr.mq_msgsize 3. EAGAIN — queue at mq_attr.mq_maxmsg Storage cap MQFAKE_MAX_MESSAGES_PER_QUEUE bumped 8 → 16 so it remains >= any maxMessages value tests pass (PMQB tests use 10). Two new MqFakeTest cases pin the natural-overflow and natural-oversize paths. Addresses CodeRabbit comment on PR #443.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1326 passed, 🙈 1 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Summary
Closes #118.
Platform/Posix/adapter unit-testing against real Linux POSIX message queues. NewTests/Support/MqFake.{h,c}mirrorsSocketFake's shape — in-memory queue registry plus one-shot failure-injection knobs (MqFake_FailNextOpen/Send/Receive(errno)). Existing 18 PMQB tests migrated; 14 self-tests pin the fake's contracts.MQ_OPEN_FAILED(Create releases the slot and returnsNullBufferfallback),SEND_FAILED(Write's mq_send check; EAGAIN/EMSGSIZE both surface),RECEIVE_FAILED(mq_receive non-EAGAIN failure; empty-queue EAGAIN stays silent — part of the polling happy path).bytesRead*(segfault → defensive false return). Test pins for use-after-Destroy (NullBuffer vtable patch) andDestroy(NULL)(UNKNOWN_DESTROY warning fallthrough).IGNORE_TEST(SolidSyslogPosixMessageQueueBuffer, HappyPathOnly)deleted — every referent now pinned or explicitly out of scope.PMQB error surface now matches the bar S12.11 set for Resolver/Datagram. Real
mq_*continues to exercise PMQB end-to-end at the BDD layer.Test plan
tunable-override-debugpreset exercises the newCreateOnMqOpenFailureReleasesSlottest at pool size 2 (slot release observable across multiple slots, not just slot 0)Follow-up — not in this PR
SolidSyslogPassthroughBufferhas an analogousIGNORE_TESTplaceholder (Tests/SolidSyslogPassthroughBufferTest.cpp:102) with a much smaller surface (no syscalls). The real residue is_Create(NULL sender). Worth a separate < ½-day story under E12.CHECK_ERROR_MESSAGE_WAS(severity, source, code)macro extraction — five tests now share the same four-line ErrorHandlerFake assertion shape. Refactor when the next slice adds a sixth.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests