Remove strerror in favor of decode_error#683
Conversation
b3c4cc3 to
310405b
Compare
06762af to
9bef9ab
Compare
3e9c3e6 to
8d5a5bc
Compare
9bef9ab to
5004e39
Compare
5004e39 to
11943a3
Compare
8d5a5bc to
4e0d6fb
Compare
| if let Err(err) = results[rindex] { | ||
| bail!("failed to take dump: {}", op.strerror(err)) | ||
| return Err(op.decode_error(err)).context("failed to take dump"); | ||
| } |
There was a problem hiding this comment.
nit, take it or leave it: now that this is no longer using bail, i feel like it could be changed to something like
results[rindex].map_err(|err| op.decode_error(err).context("failed to take dump")?;also: i was...quite struck...by the potential alternative interpretation of the string "failed to take dump". if this is not immediately obvious to you, you're probably better off that way!
There was a problem hiding this comment.
sooooo I considered writing it in that form, but vaguely decided against it for two reasons:
rustfmtwraps onto multiple lines, so it's not any shorter- Vague vibes: I feel like
?as a way to return an error – when you're not using the actual value – is a little less obvious than an explicitreturn. It's fine in a chain where the end result is used, but seems to sneaky when used as a one-character conditionalreturn.
(I have no further comment on the text of the string 🙈)
| if let Err(err) = result { | ||
| bail!( | ||
| "failed to add segment at address {base:#x} for length \ | ||
| {size}: {}", | ||
| op.strerror(*err), | ||
| ); | ||
| return Err(op.decode_error(*err)).with_context(|| { | ||
| format!( | ||
| "failed to add segment at address {base:#x} \ | ||
| for length {size}" | ||
| ) | ||
| }); |
There was a problem hiding this comment.
take it or leave it: this could ? now that it no longer uses bail!?
There was a problem hiding this comment.
Same here, I'd rather be explicit versus result.map_err(...)?
11943a3 to
6b30409
Compare
527e208 to
b85e8e1
Compare
6b30409 to
1bdb2f7
Compare
b85e8e1 to
f1176b6
Compare
1bdb2f7 to
bf11d08
Compare
f1176b6 to
df8c94d
Compare
9b94c7a to
f6196a5
Compare
824a6db to
675343f
Compare
f6196a5 to
40a3fb8
Compare
675343f to
eae72e0
Compare
418aa09 to
bc6d425
Compare
eae72e0 to
6405216
Compare
6405216 to
e6c0ea4
Compare
There was a problem hiding this comment.
Pull request overview
Consolidates Idol IPC error decoding by removing IdolOperation::strerror and routing callers through the typed IdolOperation::decode_error path, so Idol error interpretation lives in one place.
Changes:
- Removed
IdolOperation::strerrorfromhumility-idolin favor ofIdolOperation::decode_error. - Updated
humility-dump-agentto surface decoded Idol errors viaanyhow::Contextchains. - Updated
cmd/pmbusto format errors viadecode_error(...).to_string().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| humility-idol/src/lib.rs | Removes strerror API to centralize error decoding in decode_error. |
| humility-dump-agent/src/hiffy.rs | Replaces strerror usage with typed decode + contextual error propagation. |
| cmd/pmbus/src/lib.rs | Switches error-to-string formatting from strerror to decode_error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert_eq!(out.len(), 1); | ||
| if let Err(err) = &out[0] { | ||
| bail!("failed to dump task region: {}", op.strerror(*err)) | ||
| Err(op.decode_error(*err)).context("failed to dump task region") |
(staged on #682)
This is a small PR to consolidate
IpcErrordecoding into a single place (IdolOperation::decode_error).I don't try to preserve the exact error formatting; if you have opinions on what information would be useful, see #680 for where the new error type is introduced. I'd be surprised if anyone was parsing CLI output for exact strings here, but cc @adamlouis just in case.