From 32dac32a0dac8ae7c6033744b221d8ca73ca1d80 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 10 Mar 2026 11:25:34 -0400 Subject: [PATCH] Prefer using try_lookup_task for better errors --- cmd/dump/src/lib.rs | 18 +++++++++--------- cmd/hiffy/src/lib.rs | 8 ++------ cmd/jefe/src/lib.rs | 8 +++----- cmd/powershelf/src/lib.rs | 2 +- cmd/rpc/src/lib.rs | 8 ++------ humility-core/src/hubris.rs | 8 ++++---- humility-hiffy/src/lib.rs | 4 ++-- humility-idol/src/lib.rs | 6 +++--- 8 files changed, 26 insertions(+), 36 deletions(-) diff --git a/cmd/dump/src/lib.rs b/cmd/dump/src/lib.rs index 94beb575..05f23d42 100644 --- a/cmd/dump/src/lib.rs +++ b/cmd/dump/src/lib.rs @@ -427,14 +427,14 @@ fn dump_via_agent( if !subargs.simulate_dumper { bail!("--simulate-task-dump requires --simulate-dumper"); } - let ndx = match hubris.lookup_task(task) { - Some(HubrisTask::Task(ndx)) => ndx, - _ => { - bail!("invalid task \"{task}\""); + let ndx = match hubris.try_lookup_task(task)? { + HubrisTask::Task(ndx) => ndx, + HubrisTask::Kernel => { + bail!("cannot dump \"{task}\" because it is the kernel") } }; - Some(DumpTask::new(*ndx as u16, hubris.ticks(core)?)) + Some(DumpTask::new(ndx as u16, hubris.ticks(core)?)) } None => None, }; @@ -709,10 +709,10 @@ fn dump_task_via_agent( let mut agent = get_dump_agent(hubris, core, subargs)?; let task = subargs.task.as_ref().unwrap(); - let ndx = match hubris.lookup_task(task) { - Some(HubrisTask::Task(ndx)) => *ndx, - _ => { - bail!("invalid task \"{task}\""); + let ndx = match hubris.try_lookup_task(task)? { + HubrisTask::Task(ndx) => ndx, + HubrisTask::Kernel => { + bail!("cannot dump \"{task}\" because it is the kernel") } }; if ndx == 0 { diff --git a/cmd/hiffy/src/lib.rs b/cmd/hiffy/src/lib.rs index 67af4eff..13d8332e 100644 --- a/cmd/hiffy/src/lib.rs +++ b/cmd/hiffy/src/lib.rs @@ -46,7 +46,7 @@ //! use ::idol::syntax::{Operation, Reply}; -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, bail}; use clap::{CommandFactory, Parser}; use humility::hubris::*; use humility::warn; @@ -259,11 +259,7 @@ fn hiffy(context: &mut ExecutionContext) -> Result<()> { } let task = match subargs.task { - Some(task) => Some( - hubris - .lookup_task(&task) - .ok_or_else(|| anyhow!("unknown task \"{}\"", task))?, - ), + Some(task) => Some(hubris.try_lookup_task(&task)?), None => None, }; diff --git a/cmd/jefe/src/lib.rs b/cmd/jefe/src/lib.rs index f61e0c03..f425b7cc 100644 --- a/cmd/jefe/src/lib.rs +++ b/cmd/jefe/src/lib.rs @@ -94,7 +94,7 @@ //! As with tasks held by `--hold`, use `--release`/`-r` to set the task back to //! normal, or `--start`/`-s` to run it once but catch the next fault. -use anyhow::{Result, anyhow, bail}; +use anyhow::{Result, bail}; use clap::{CommandFactory, Parser}; use humility::hubris::*; use humility_cli::{ExecutionContext, Subcommand}; @@ -150,16 +150,14 @@ fn jefe(context: &mut ExecutionContext) -> Result<()> { bail!("one of fault, start, hold, or release must be specified"); }; - let task = hubris - .lookup_task(&subargs.task) - .ok_or_else(|| anyhow!("couldn't find task {}", subargs.task))?; + let task = hubris.try_lookup_task(&subargs.task)?; let id = match task { HubrisTask::Kernel => { bail!("cannot change disposition of kernel"); } HubrisTask::Task(id) => { - if let Some(id) = NonZeroU32::new(*id) { + if let Some(id) = NonZeroU32::new(id) { id } else { bail!("cannot change disposition of supervisor task"); diff --git a/cmd/powershelf/src/lib.rs b/cmd/powershelf/src/lib.rs index 3e2daa28..d42c75ad 100644 --- a/cmd/powershelf/src/lib.rs +++ b/cmd/powershelf/src/lib.rs @@ -54,7 +54,7 @@ fn lookup_operation_enum(hubris: &HubrisArchive) -> Result<&HubrisEnum> { anyhow!("missing `power` task - is this a PSC archive?") })?; - let module = hubris.lookup_module(*power_task)?; + let module = hubris.lookup_module(power_task)?; // `Operation` matches multiple enums, and `task_power_api::Operation` // doesn't show up alone, but `Option` does! Find diff --git a/cmd/rpc/src/lib.rs b/cmd/rpc/src/lib.rs index 627fe4be..d25a4322 100644 --- a/cmd/rpc/src/lib.rs +++ b/cmd/rpc/src/lib.rs @@ -389,7 +389,7 @@ impl<'a> RpcClient<'a> { ) })?; let rpc_reply_type = hubris - .lookup_module(*rpc_task)? + .lookup_module(rpc_task)? .lookup_enum_byname(hubris, "RpcReply")? .ok_or_else(|| anyhow!("can't find RpcReply"))?; @@ -550,11 +550,7 @@ fn rpc_run(context: &mut ExecutionContext) -> Result<()> { } let task = match &subargs.task { - Some(task) => Some( - hubris - .lookup_task(task) - .ok_or_else(|| anyhow!("unknown task \"{}\"", task))?, - ), + Some(task) => Some(hubris.try_lookup_task(task)?), None => None, }; diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 157f1617..ae89923c 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1976,15 +1976,15 @@ impl HubrisArchive { self.modules.values() } - pub fn lookup_task(&self, name: &str) -> Option<&HubrisTask> { - self.tasks.get(name) + pub fn lookup_task(&self, name: &str) -> Option { + self.tasks.get(name).copied() } /// Tries to look up a task by name, returning an error with similar names pub fn try_lookup_task(&self, name: &str) -> Result { self.tasks .get(name) - .cloned() + .copied() .ok_or_else(|| self.task_name_suggestion(name)) } @@ -6684,7 +6684,7 @@ impl ExternRegions { for (name, task) in &config.tasks { if task.extern_regions.is_some() { - set.insert(*hubris.lookup_task(name).unwrap()); + set.insert(hubris.lookup_task(name).unwrap()); } } diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index 6ae69854..7d6e1a2f 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -368,7 +368,7 @@ impl<'a> HiffyContext<'a> { Some( hubris - .lookup_module(*rpc_task)? + .lookup_module(rpc_task)? .lookup_enum_byname(hubris, "RpcReply")? .ok_or_else(|| anyhow!("failed to find RpcReply"))?, ) @@ -984,7 +984,7 @@ impl<'a> HiffyContext<'a> { None } - HubrisTask::Task(i) => Some(*i), + HubrisTask::Task(i) => Some(i), } }); diff --git a/humility-idol/src/lib.rs b/humility-idol/src/lib.rs index 1a122b80..234d906f 100644 --- a/humility-idol/src/lib.rs +++ b/humility-idol/src/lib.rs @@ -62,7 +62,7 @@ impl<'a> IdolOperation<'a> { hubris: &'a HubrisArchive, interface: &str, operation: &str, - task: Option<&HubrisTask>, + task: Option, ) -> Result { let (task, op, code) = lookup(hubris, interface, operation, task)?; let name = (interface.to_string(), operation.to_string()); @@ -309,7 +309,7 @@ fn lookup<'a>( hubris: &'a HubrisArchive, interface: &str, operation: &str, - target: Option<&HubrisTask>, + target: Option, ) -> Result<(HubrisTask, &'a Operation, u16)> { // // Find this interface and its operation. @@ -317,7 +317,7 @@ fn lookup<'a>( let mut rval = None; let tasks = match target { - Some(task) => vec![*task], + Some(task) => vec![task], None => { (0..hubris.ntasks()).map(|t| HubrisTask::Task(t as u32)).collect() }