libmoq: expose moq_error(), stop logging FFI errors#1586
Conversation
WalkthroughAdds per-thread FFI error storage and access: imports and a thread-local LAST_ERROR with recording/reader helpers, extends ReturnCode with error() and updates implementations, records the thread’s last error in enter and OnStatus::call before computing the numeric return code so callbacks can call moq_error(), exports a C-ABI moq_error() returning a NUL-terminated pointer (or NULL), and updates docs plus unit tests verifying the reported message. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
kixelated
force-pushed
the
claude/libmoq-last-error
branch
from
9696ac3 to
3bba07a
Compare
kixelated
changed the title
FFI functions return only a numeric code, so the human-readable reason was reachable only through `Error::code()`'s unconditional `tracing::error!`. That had two problems: routine lookup misses (e.g. closing a handle whose background task ended a hair before the call, as when a session drops) were logged at error level, and the reason was invisible unless the caller had installed a tracing subscriber via `moq_log_level`. Capture the reason in a thread-local on every error and expose it via a new `moq_error() -> *const c_char` (errno/strerror style: valid until the next call on the same thread, meaningful only after a negative return). `OnStatus::call` computes the code (which records the reason) before invoking the callback on the same thread, so a callback receiving a negative status can read `moq_error()` for the reason. Drop the logging from `code()` entirely; `moq_log_level` still governs the moq-net/QUIC internal logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kixelated
force-pushed
the
claude/libmoq-last-error
branch
from
3bba07a to
8ee3e6e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/libmoq/src/ffi.rs (1)
113-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate
moq_error()for synthesizedInvalidCodefailures.These
code()impls can manufactureError::InvalidCodefrom anOk(...), but theirerror()view still returnsNone. With the new boundary logic, that means callers can receive-15whilemoq_error()stays null or stale.Also applies to: 127-137, 166-169
🤖 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 `@rs/libmoq/src/ffi.rs` around lines 113 - 124, The ReturnCode.error() implementations must return a synthesized Error when code() fabricates Error::InvalidCode from an Ok value; update the error() for the impls of ReturnCode on Result<i32, Error>, Result<i64, Error> and Result<Option<i32>, Error> so that when self is Ok(code) and the numeric value is negative (or Option::Some negative) you return Some(&Error::InvalidCode) instead of None — keep returning the underlying Err(e) as Some(e) when self is Err, and return None only for legitimate non-error Ok values; reference the ReturnCode trait methods code() and error(), and the specific impl blocks for Result<i32, Error>, Result<i64, Error>, and Result<Option<i32>, Error> when making the change.
🤖 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 `@doc/lib/c/index.md`:
- Line 64: The fprintf call uses moq_error() directly which can return NULL and
cause undefined behavior; update the call that prints the error (the fprintf in
the C example) to guard moq_error() with a null-safe fallback (e.g., use
moq_error() ? moq_error() : "(null)") so the "%s" formatter never receives a
NULL pointer.
In `@rs/libmoq/src/ffi.rs`:
- Around line 189-203: The record_error function must clear the thread-local
LAST_ERROR when there is no error to uphold the last_error_ptr lifetime
contract; modify record_error (and use LAST_ERROR.with) so that if ret.error()
is None it sets *cell.borrow_mut() = None (and returns), otherwise proceed to
create a CString and store it as before; ensure last_error_ptr still returns
null when LAST_ERROR is None.
---
Outside diff comments:
In `@rs/libmoq/src/ffi.rs`:
- Around line 113-124: The ReturnCode.error() implementations must return a
synthesized Error when code() fabricates Error::InvalidCode from an Ok value;
update the error() for the impls of ReturnCode on Result<i32, Error>,
Result<i64, Error> and Result<Option<i32>, Error> so that when self is Ok(code)
and the numeric value is negative (or Option::Some negative) you return
Some(&Error::InvalidCode) instead of None — keep returning the underlying Err(e)
as Some(e) when self is Err, and return None only for legitimate non-error Ok
values; reference the ReturnCode trait methods code() and error(), and the
specific impl blocks for Result<i32, Error>, Result<i64, Error>, and
Result<Option<i32>, Error> when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3c2af1b-21ec-45c4-b638-b30622405dd4
📒 Files selected for processing (5)
doc/lib/c/index.mdrs/libmoq/src/api.rsrs/libmoq/src/error.rsrs/libmoq/src/ffi.rsrs/libmoq/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/libmoq/src/api.rs
- rs/libmoq/src/test.rs
| ```c | ||
| int rc = moq_consume_catalog_close(catalog); | ||
| if (rc < 0) { | ||
| fprintf(stderr, "close failed: %s\n", moq_error()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C, is passing a NULL pointer to printf/fprintf with the "%s" format specifier defined behavior by the C standard?
💡 Result:
Passing a NULL pointer as the argument for the %s conversion in printf/fprintf is undefined behavior by the C standard. This is because %s requires a pointer to a null-terminated character array (i.e., a valid string pointer), and the standard’s general rule for library functions says that passing a null pointer as an argument yields undefined behavior unless the specific function explicitly states otherwise.[1][2][3] Notes on common implementations: many C libraries print something like “(null)” as an extension, but that behavior is not guaranteed by the C standard and may vary across implementations.[4][2][1]
Citations:
- 1: https://stackoverflow.com/questions/11589342/what-is-the-behavior-of-printing-null-with-printfs-s-specifier
- 2: https://www.trust-in-soft.com/resources/blogs/2019-07-09-printing-a-null-pointer-with-s-is-undefined-behavior
- 3: https://stackoverflow.com/questions/44996471/printing-null-pointers-with-p-is-undefined-behavior
- 4: https://libc.llvm.org/dev/printf_behavior.html
Guard moq_error() before "%s" formatting in the C example.
moq_error() can return NULL; passing a NULL pointer to fprintf(..., "%s", ...) is undefined behavior. Update the snippet to use a null-safe fallback (e.g., moq_error() ? moq_error() : "(null)").
fprintf(stderr, "close failed: %s\n", moq_error());🤖 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 `@doc/lib/c/index.md` at line 64, The fprintf call uses moq_error() directly
which can return NULL and cause undefined behavior; update the call that prints
the error (the fprintf in the C example) to guard moq_error() with a null-safe
fallback (e.g., use moq_error() ? moq_error() : "(null)") so the "%s" formatter
never receives a NULL pointer.
| fn record_error<C: ReturnCode>(ret: &C) { | ||
| let Some(err) = ret.error() else { return }; | ||
| // CString::new fails only on an interior NUL, which our messages never | ||
| // contain; skip storing rather than truncating if it ever happens. | ||
| if let Ok(msg) = CString::new(err.to_string()) { | ||
| LAST_ERROR.with(|cell| *cell.borrow_mut() = Some(msg)); | ||
| } | ||
| } | ||
|
|
||
| /// Pointer to this thread's last error message, or null if none was recorded. | ||
| /// | ||
| /// The pointer is valid until the next libmoq call on the same thread. | ||
| pub fn last_error_ptr() -> *const c_char { | ||
| LAST_ERROR.with(|cell| cell.borrow().as_ref().map_or(std::ptr::null(), |msg| msg.as_ptr())) | ||
| } |
There was a problem hiding this comment.
Clear LAST_ERROR on non-error calls.
record_error() returns early when ret.error() is None, so a successful libmoq call leaves the previous thread-local message intact. That breaks the Line 200 lifetime contract and lets moq_error() report a stale reason after success.
Suggested fix
fn record_error<C: ReturnCode>(ret: &C) {
- let Some(err) = ret.error() else { return };
- // CString::new fails only on an interior NUL, which our messages never
- // contain; skip storing rather than truncating if it ever happens.
- if let Ok(msg) = CString::new(err.to_string()) {
- LAST_ERROR.with(|cell| *cell.borrow_mut() = Some(msg));
- }
+ let msg = ret
+ .error()
+ .and_then(|err| CString::new(err.to_string()).ok());
+
+ LAST_ERROR.with(|cell| *cell.borrow_mut() = msg);
}🤖 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 `@rs/libmoq/src/ffi.rs` around lines 189 - 203, The record_error function must
clear the thread-local LAST_ERROR when there is no error to uphold the
last_error_ptr lifetime contract; modify record_error (and use LAST_ERROR.with)
so that if ret.error() is None it sets *cell.borrow_mut() = None (and returns),
otherwise proceed to create a CString and store it as before; ensure
last_error_ptr still returns null when LAST_ERROR is None.
1 participant
Summary
libmoq's FFI returns only a numeric code, so the human-readable reason for a failure was reachable only through
Error::code()'s unconditionaltracing::error!. Two problems with that:*_closecall, so the close returns a benign*NotFoundthat got logged as an error.moq_log_level. With no subscriber (the default), a caller saw just thei32and lost the detail the code can't carry (which URL failed to parse, why a decode failed, etc.).This adds a proper retrieval path and removes the logging:
moq_error() -> *const c_char, errno/strerror style. Returns the reason for the most recent failed call on the calling thread, recorded into a thread-local. The pointer is valid until the next libmoq call on that thread and is only meaningful after a negative return (check the code first).moq_error()for the reason. Read it from inside the callback if you marshal to another thread afterward.code()side effect.ReturnCodegains anerror()accessor;enterandOnStatus::callcall arecord_error()helper before producing the numeric code, socode()stays a pureError -> i32conversion. The unconditional logging is gone;moq_log_levelstill governs the moq-net / QUIC internal logs.The
*_closerace itself is unchanged: a close landing after the task self-terminated still returns its truthful negative code. That's correct (there was nothing left to close); it just no longer spams the log, and the reason is now retrievable on demand. (This supersedes earlier explorations of idempotent close and typed handles, which were dropped as over-engineering for what is fundamentally a logging/observability gap.)Changes
rs/libmoq/src/ffi.rs— thread-localLAST_ERROR+record_error()(boundary helper) +last_error_ptr();ReturnCode::error()accessor;enterandOnStatus::callrecord before converting to a code.rs/libmoq/src/error.rs—Error::code()is a pure conversion again (no logging);error()returnsSome(self).rs/libmoq/src/api.rs—moq_error()(regeneratesconst char *moq_error(void);inmoq.hvia cbindgen).rs/libmoq/src/test.rs—last_error_reports_reason(sync path) andlast_error_set_before_callback(drivesOnStatus::calldirectly to assert the reason is visible inside a negative-status callback).doc/lib/c/index.md— new "Error handling" section.Test plan
cargo build -p libmoq(regeneratesmoq.hwithmoq_error)cargo test -p libmoq— 23 passedcargo clippy -p libmoq --all-targets— cleancargo fmt -p libmoq(all via
nix developto match CI's pinned toolchain)🤖 Generated with Claude Code