From f5a79642ba26e8ff7c52260c8b47e5bfd48d5334 Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Fri, 3 Nov 2023 21:48:31 +0000 Subject: [PATCH 1/8] CmdError error messages should provide more information --- common/src/cmd.rs | 4 +- dev-tools/omicron-dev/src/bin/omicron-dev.rs | 2 +- gateway/src/bin/mgs.rs | 93 ++++++++++--------- .../src/bin/installinator-artifactd.rs | 7 +- nexus/src/bin/nexus.rs | 9 +- oximeter/collector/src/bin/oximeter.rs | 19 ++-- sled-agent/src/bin/sled-agent-sim.rs | 10 +- sled-agent/src/bin/sled-agent.rs | 24 ++--- sp-sim/src/bin/sp-sim.rs | 8 +- wicketd/src/bin/wicketd.rs | 41 ++++---- 10 files changed, 118 insertions(+), 99 deletions(-) diff --git a/common/src/cmd.rs b/common/src/cmd.rs index d92ebe4c98..f7ede8d127 100644 --- a/common/src/cmd.rs +++ b/common/src/cmd.rs @@ -13,7 +13,7 @@ pub enum CmdError { /// incorrect command-line arguments Usage(String), /// all other errors - Failure(String), + Failure(anyhow::Error), } /// Exits the current process on a fatal error. @@ -26,7 +26,7 @@ pub fn fatal(cmd_error: CmdError) -> ! { .unwrap_or("command"); let (exit_code, message) = match cmd_error { CmdError::Usage(m) => (2, m), - CmdError::Failure(m) => (1, m), + CmdError::Failure(e) => (1, format!("{e:?}")), }; eprintln!("{}: {}", arg0, message); exit(exit_code); diff --git a/dev-tools/omicron-dev/src/bin/omicron-dev.rs b/dev-tools/omicron-dev/src/bin/omicron-dev.rs index 66778d96e7..dc0ae05f25 100644 --- a/dev-tools/omicron-dev/src/bin/omicron-dev.rs +++ b/dev-tools/omicron-dev/src/bin/omicron-dev.rs @@ -34,7 +34,7 @@ async fn main() -> Result<(), anyhow::Error> { OmicronDb::CertCreate { ref args } => cmd_cert_create(args).await, }; if let Err(error) = result { - fatal(CmdError::Failure(format!("{:#}", error))); + fatal(CmdError::Failure(error.into())); } Ok(()) } diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs index 81b10ef669..79fd025867 100644 --- a/gateway/src/bin/mgs.rs +++ b/gateway/src/bin/mgs.rs @@ -4,6 +4,7 @@ //! Executable program to run gateway, the management gateway service +use anyhow::{anyhow, Context}; use clap::Parser; use futures::StreamExt; use omicron_common::cmd::{fatal, CmdError}; @@ -70,26 +71,24 @@ async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); match args { - Args::Openapi => run_openapi().map_err(CmdError::Failure), + Args::Openapi => { + run_openapi().map_err(|e| CmdError::Failure(anyhow!("{e}"))) + } Args::Run { config_file_path, id_and_address_from_smf, id, address, } => { - let config = Config::from_file(&config_file_path).map_err(|e| { - CmdError::Failure(format!( - "failed to parse {}: {}", - config_file_path.display(), - e - )) - })?; - - let mut signals = Signals::new([signal::SIGUSR1]).map_err(|e| { - CmdError::Failure(format!( - "failed to set up signal handler: {e}" - )) - })?; + let config = Config::from_file(&config_file_path) + .with_context(|| { + format!("failed to parse {}", config_file_path.display()) + }) + .map_err(CmdError::Failure)?; + + let mut signals = Signals::new([signal::SIGUSR1]) + .context("failed to set up signal handler") + .map_err(CmdError::Failure)?; let (id, addresses, rack_id) = if id_and_address_from_smf { let config = read_smf_config()?; @@ -102,8 +101,9 @@ async fn do_run() -> Result<(), CmdError> { (id.unwrap(), vec![address.unwrap()], rack_id) }; let args = MgsArguments { id, addresses, rack_id }; - let mut server = - start_server(config, args).await.map_err(CmdError::Failure)?; + let mut server = start_server(config, args) + .await + .map_err(|e| CmdError::Failure(anyhow!(e)))?; loop { tokio::select! { @@ -111,18 +111,13 @@ async fn do_run() -> Result<(), CmdError> { Some(signal::SIGUSR1) => { let new_config = read_smf_config()?; if new_config.id != id { - return Err(CmdError::Failure( - "cannot change server ID on refresh" - .to_string() - )); + return Err(CmdError::Failure(anyhow!("cannot change server ID on refresh"))); } server.set_rack_id(new_config.rack_id); server .adjust_dropshot_addresses(&new_config.addresses) .await - .map_err(|err| CmdError::Failure( - format!("config refresh failed: {err}") - ))?; + .map_err(|err| CmdError::Failure(anyhow!("config refresh failed: {err}")))?; } // We only register `SIGUSR1` and never close the // handle, so we never expect `None` or any other @@ -130,7 +125,7 @@ async fn do_run() -> Result<(), CmdError> { _ => unreachable!("invalid signal: {signal:?}"), }, result = server.wait_for_finish() => { - return result.map_err(CmdError::Failure) + return result.map_err(|err| CmdError::Failure(anyhow!("{err}"))) } } } @@ -141,7 +136,7 @@ async fn do_run() -> Result<(), CmdError> { #[cfg(target_os = "illumos")] fn read_smf_config() -> Result { fn scf_to_cmd_err(err: illumos_utils::scf::ScfError) -> CmdError { - CmdError::Failure(err.to_string()) + CmdError::Failure(err.into()) } use illumos_utils::scf::ScfHandle; @@ -165,12 +160,14 @@ fn read_smf_config() -> Result { let prop_id = config.value_as_string(PROP_ID).map_err(scf_to_cmd_err)?; - let prop_id = Uuid::try_parse(&prop_id).map_err(|err| { - CmdError::Failure(format!( - "failed to parse `{CONFIG_PG}/{PROP_ID}` \ - ({prop_id:?}) as a UUID: {err}" - )) - })?; + let prop_id = Uuid::try_parse(&prop_id) + .with_context(|| { + format!( + "failed to parse `{CONFIG_PG}/{PROP_ID}` ({prop_id:?}) as a \ + UUID" + ) + }) + .map_err(CmdError::Failure)?; let prop_rack_id = config.value_as_string(PROP_RACK_ID).map_err(scf_to_cmd_err)?; @@ -178,12 +175,16 @@ fn read_smf_config() -> Result { let rack_id = if prop_rack_id == "unknown" { None } else { - Some(Uuid::try_parse(&prop_rack_id).map_err(|err| { - CmdError::Failure(format!( - "failed to parse `{CONFIG_PG}/{PROP_RACK_ID}` \ - ({prop_rack_id:?}) as a UUID: {err}" - )) - })?) + Some( + Uuid::try_parse(&prop_rack_id) + .with_context(|| { + format!( + "failed to parse `{CONFIG_PG}/{PROP_RACK_ID}` \ + ({prop_rack_id:?}) as a UUID" + ) + }) + .map_err(CmdError::Failure)?, + ) }; let prop_addr = @@ -192,16 +193,20 @@ fn read_smf_config() -> Result { let mut addresses = Vec::with_capacity(prop_addr.len()); for addr in prop_addr { - addresses.push(addr.parse().map_err(|err| { - CmdError::Failure(format!( - "failed to parse `{CONFIG_PG}/{PROP_ADDR}` \ - ({addr:?}) as a socket address: {err}" - )) - })?); + addresses.push( + addr.parse() + .with_context(|| { + format!( + "failed to parse `{CONFIG_PG}/{PROP_ADDR}` ({addr:?}) \ + as a socket address" + ) + }) + .map_err(CmdError::Failure)?, + ); } if addresses.is_empty() { - Err(CmdError::Failure(format!( + Err(CmdError::Failure(anyhow!( "no addresses specified by `{CONFIG_PG}/{PROP_ADDR}`" ))) } else { diff --git a/installinator-artifactd/src/bin/installinator-artifactd.rs b/installinator-artifactd/src/bin/installinator-artifactd.rs index b09dc82acd..abe63bbe31 100644 --- a/installinator-artifactd/src/bin/installinator-artifactd.rs +++ b/installinator-artifactd/src/bin/installinator-artifactd.rs @@ -29,10 +29,9 @@ fn do_run() -> Result<(), CmdError> { match args { Args::Openapi => { installinator_artifactd::run_openapi().map_err(|error| { - CmdError::Failure(format!( - "failed to generate OpenAPI spec: {:?}", - error - )) + CmdError::Failure( + error.context("failed to generate OpenAPI spec"), + ) }) } } diff --git a/nexus/src/bin/nexus.rs b/nexus/src/bin/nexus.rs index 76e4f7aaca..c283af7313 100644 --- a/nexus/src/bin/nexus.rs +++ b/nexus/src/bin/nexus.rs @@ -10,6 +10,7 @@ // - General networking and runtime tuning for availability and security: see // omicron#2184, omicron#2414. +use anyhow::anyhow; use clap::Parser; use omicron_common::cmd::fatal; use omicron_common::cmd::CmdError; @@ -53,13 +54,13 @@ async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); let config = Config::from_file(args.config_file_path) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; if args.openapi { - run_openapi_external().map_err(CmdError::Failure) + run_openapi_external().map_err(|err| CmdError::Failure(anyhow!(err))) } else if args.openapi_internal { - run_openapi_internal().map_err(CmdError::Failure) + run_openapi_internal().map_err(|err| CmdError::Failure(anyhow!(err))) } else { - run_server(&config).await.map_err(CmdError::Failure) + run_server(&config).await.map_err(|err| CmdError::Failure(anyhow!(err))) } } diff --git a/oximeter/collector/src/bin/oximeter.rs b/oximeter/collector/src/bin/oximeter.rs index 8c4bf0e27c..5eb666fcd0 100644 --- a/oximeter/collector/src/bin/oximeter.rs +++ b/oximeter/collector/src/bin/oximeter.rs @@ -6,6 +6,7 @@ // Copyright 2023 Oxide Computer Company +use anyhow::{anyhow, Context}; use clap::Parser; use omicron_common::cmd::fatal; use omicron_common::cmd::CmdError; @@ -132,7 +133,9 @@ async fn main() { async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); match args { - Args::Openapi => run_openapi().map_err(CmdError::Failure), + Args::Openapi => { + run_openapi().map_err(|err| CmdError::Failure(anyhow!("{err}"))) + } Args::Run { config_file, id, address } => { let config = Config::from_file(config_file).unwrap(); let args = OximeterArguments { id, address }; @@ -141,13 +144,15 @@ async fn do_run() -> Result<(), CmdError> { .unwrap() .serve_forever() .await - .map_err(|e| CmdError::Failure(e.to_string())) + .context("Failed to create oximeter") + .map_err(CmdError::Failure) } Args::Standalone { id, address, nexus, clickhouse, log_level } => { // Start the standalone Nexus server, for registration of both the // collector and producers. let nexus_server = StandaloneNexus::new(nexus.into(), log_level) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .context("Failed to create nexus") + .map_err(CmdError::Failure)?; let args = OximeterArguments { id, address }; Oximeter::new_standalone( nexus_server.log(), @@ -159,10 +164,10 @@ async fn do_run() -> Result<(), CmdError> { .unwrap() .serve_forever() .await - .map_err(|e| CmdError::Failure(e.to_string())) - } - Args::StandaloneOpenapi => { - run_standalone_openapi().map_err(CmdError::Failure) + .context("Failed to create standalone oximeter") + .map_err(CmdError::Failure) } + Args::StandaloneOpenapi => run_standalone_openapi() + .map_err(|err| CmdError::Failure(anyhow!(err))), } } diff --git a/sled-agent/src/bin/sled-agent-sim.rs b/sled-agent/src/bin/sled-agent-sim.rs index adba8eab6b..1ffc9420a8 100644 --- a/sled-agent/src/bin/sled-agent-sim.rs +++ b/sled-agent/src/bin/sled-agent-sim.rs @@ -95,8 +95,8 @@ async fn main() { async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); - let tmp = camino_tempfile::tempdir() - .map_err(|e| CmdError::Failure(e.to_string()))?; + let tmp = + camino_tempfile::tempdir().map_err(|e| CmdError::Failure(e.into()))?; let config = Config { id: args.uuid, sim_mode: args.sim_mode, @@ -125,10 +125,10 @@ async fn do_run() -> Result<(), CmdError> { (Some(cert_path), Some(key_path)) => { let cert_bytes = std::fs::read_to_string(&cert_path) .with_context(|| format!("read {:?}", &cert_path)) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; let key_bytes = std::fs::read_to_string(&key_path) .with_context(|| format!("read {:?}", &key_path)) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; Some(NexusTypes::Certificate { cert: cert_bytes, key: key_bytes }) } _ => { @@ -147,5 +147,5 @@ async fn do_run() -> Result<(), CmdError> { run_standalone_server(&config, &rss_args) .await - .map_err(|e| CmdError::Failure(e.to_string())) + .map_err(|e| CmdError::Failure(e.into())) } diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index 5764bf8309..0843484900 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -4,6 +4,7 @@ //! Executable program to run the sled agent +use anyhow::anyhow; use camino::Utf8PathBuf; use clap::{Parser, Subcommand}; use omicron_common::cmd::fatal; @@ -51,16 +52,14 @@ async fn do_run() -> Result<(), CmdError> { match args { Args::Openapi(flavor) => match flavor { - OpenapiFlavor::Sled => { - sled_server::run_openapi().map_err(CmdError::Failure) - } - OpenapiFlavor::Bootstrap => { - bootstrap_server::run_openapi().map_err(CmdError::Failure) - } + OpenapiFlavor::Sled => sled_server::run_openapi() + .map_err(|err| CmdError::Failure(anyhow!("{err}"))), + OpenapiFlavor::Bootstrap => bootstrap_server::run_openapi() + .map_err(|err| CmdError::Failure(anyhow!("{err}"))), }, Args::Run { config_path } => { let config = SledConfig::from_file(&config_path) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; // - Sled agent starts with the normal config file - typically // called "config.toml". @@ -83,7 +82,7 @@ async fn do_run() -> Result<(), CmdError> { let rss_config = if rss_config_path.exists() { Some( RssConfig::from_file(rss_config_path) - .map_err(|e| CmdError::Failure(e.to_string()))?, + .map_err(|e| CmdError::Failure(e.into()))?, ) } else { None @@ -91,7 +90,7 @@ async fn do_run() -> Result<(), CmdError> { let server = bootstrap_server::Server::start(config) .await - .map_err(|err| CmdError::Failure(format!("{err:#}")))?; + .map_err(|err| CmdError::Failure(err.into()))?; // If requested, automatically supply the RSS configuration. // @@ -103,12 +102,15 @@ async fn do_run() -> Result<(), CmdError> { // abandon the server. Ok(_) | Err(RssAccessError::AlreadyInitialized) => {} Err(e) => { - return Err(CmdError::Failure(e.to_string())); + return Err(CmdError::Failure(e.into())); } } } - server.wait_for_finish().await.map_err(CmdError::Failure)?; + server + .wait_for_finish() + .await + .map_err(|err| CmdError::Failure(anyhow!("{err}")))?; Ok(()) } diff --git a/sp-sim/src/bin/sp-sim.rs b/sp-sim/src/bin/sp-sim.rs index 76cb851528..00261cbc3f 100644 --- a/sp-sim/src/bin/sp-sim.rs +++ b/sp-sim/src/bin/sp-sim.rs @@ -27,14 +27,14 @@ async fn main() { async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); let config = Config::from_file(args.config_file_path) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; - let log = sp_sim::logger(&config) - .map_err(|e| CmdError::Failure(e.to_string()))?; + let log = + sp_sim::logger(&config).map_err(|e| CmdError::Failure(e.into()))?; let _rack = SimRack::start(&config, &log) .await - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; // for now, do nothing except let the spawned tasks run. in the future // (or when used as a library), the expectation is that a caller can diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index 2e6d51c0f0..c615ae3da4 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -4,6 +4,7 @@ //! Executable for wicketd: technician port based management service +use anyhow::{anyhow, Context}; use clap::Parser; use omicron_common::{ address::Ipv6Subnet, @@ -69,7 +70,9 @@ async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); match args { - Args::Openapi => run_openapi().map_err(CmdError::Failure), + Args::Openapi => { + run_openapi().map_err(|err| CmdError::Failure(anyhow!("{err}"))) + } Args::Run { config_file_path, address, @@ -82,10 +85,10 @@ async fn do_run() -> Result<(), CmdError> { } => { let baseboard = if let Some(baseboard_file) = baseboard_file { let baseboard_file = std::fs::read_to_string(baseboard_file) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; let baseboard: Baseboard = serde_json::from_str(&baseboard_file) - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; // TODO-correctness `Baseboard::unknown()` is slated for removal // after some refactoring in sled-agent, at which point we'll @@ -100,19 +103,17 @@ async fn do_run() -> Result<(), CmdError> { None }; - let config = Config::from_file(&config_file_path).map_err(|e| { - CmdError::Failure(format!( - "failed to parse {}: {}", - config_file_path.display(), - e - )) - })?; + let config = Config::from_file(&config_file_path) + .with_context(|| { + format!("failed to parse {}", config_file_path.display()) + }) + .map_err(CmdError::Failure)?; let rack_subnet = match rack_subnet { Some(addr) => Some(Ipv6Subnet::new(addr)), None if read_smf_config => { let smf_values = SmfConfigValues::read_current() - .map_err(|e| CmdError::Failure(e.to_string()))?; + .map_err(|e| CmdError::Failure(e.into()))?; smf_values.rack_subnet } None => None, @@ -126,12 +127,18 @@ async fn do_run() -> Result<(), CmdError> { baseboard, rack_subnet, }; - let log = config.log.to_logger("wicketd").map_err(|msg| { - CmdError::Failure(format!("initializing logger: {}", msg)) - })?; - let server = - Server::start(log, args).await.map_err(CmdError::Failure)?; - server.wait_for_finish().await.map_err(CmdError::Failure) + let log = config + .log + .to_logger("wicketd") + .context("failed to initialize logger") + .map_err(CmdError::Failure)?; + let server = Server::start(log, args) + .await + .map_err(|err| CmdError::Failure(anyhow!("{err}")))?; + server + .wait_for_finish() + .await + .map_err(|err| CmdError::Failure(anyhow!("{err}"))) } } } From 32082dd04c898f08aa88a3e82fff8c2d18ddc23b Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Mon, 6 Nov 2023 12:44:46 -0500 Subject: [PATCH 2/8] Fix non illumos platforms --- gateway/src/bin/mgs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs index 79fd025867..8754c26c30 100644 --- a/gateway/src/bin/mgs.rs +++ b/gateway/src/bin/mgs.rs @@ -216,7 +216,7 @@ fn read_smf_config() -> Result { #[cfg(not(target_os = "illumos"))] fn read_smf_config() -> Result { - Err(CmdError::Failure( - "SMF configuration only available on illumos".to_string(), - )) + Err(CmdError::Failure(anyhow!( + "SMF configuration only available on illumos" + ))) } From f2a4fe5096c217d8c44fe43296565df9d99605cb Mon Sep 17 00:00:00 2001 From: Michael Zeller Date: Mon, 6 Nov 2023 13:07:55 -0500 Subject: [PATCH 3/8] Apply jgallagher suggestions from code review Co-authored-by: John Gallagher --- gateway/src/bin/mgs.rs | 4 ++-- oximeter/collector/src/bin/oximeter.rs | 2 +- sled-agent/src/bin/sled-agent.rs | 6 +++--- wicketd/src/bin/wicketd.rs | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs index 8754c26c30..b83578b384 100644 --- a/gateway/src/bin/mgs.rs +++ b/gateway/src/bin/mgs.rs @@ -72,7 +72,7 @@ async fn do_run() -> Result<(), CmdError> { match args { Args::Openapi => { - run_openapi().map_err(|e| CmdError::Failure(anyhow!("{e}"))) + run_openapi().map_err(|e| CmdError::Failure(anyhow!(e))) } Args::Run { config_file_path, @@ -125,7 +125,7 @@ async fn do_run() -> Result<(), CmdError> { _ => unreachable!("invalid signal: {signal:?}"), }, result = server.wait_for_finish() => { - return result.map_err(|err| CmdError::Failure(anyhow!("{err}"))) + return result.map_err(|err| CmdError::Failure(anyhow!(err))) } } } diff --git a/oximeter/collector/src/bin/oximeter.rs b/oximeter/collector/src/bin/oximeter.rs index 5eb666fcd0..d97ae5e72e 100644 --- a/oximeter/collector/src/bin/oximeter.rs +++ b/oximeter/collector/src/bin/oximeter.rs @@ -134,7 +134,7 @@ async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); match args { Args::Openapi => { - run_openapi().map_err(|err| CmdError::Failure(anyhow!("{err}"))) + run_openapi().map_err(|err| CmdError::Failure(anyhow!(err))) } Args::Run { config_file, id, address } => { let config = Config::from_file(config_file).unwrap(); diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index 0843484900..f4d654c2c1 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -53,9 +53,9 @@ async fn do_run() -> Result<(), CmdError> { match args { Args::Openapi(flavor) => match flavor { OpenapiFlavor::Sled => sled_server::run_openapi() - .map_err(|err| CmdError::Failure(anyhow!("{err}"))), + .map_err(|err| CmdError::Failure(anyhow!(err))), OpenapiFlavor::Bootstrap => bootstrap_server::run_openapi() - .map_err(|err| CmdError::Failure(anyhow!("{err}"))), + .map_err(|err| CmdError::Failure(anyhow!(err))), }, Args::Run { config_path } => { let config = SledConfig::from_file(&config_path) @@ -110,7 +110,7 @@ async fn do_run() -> Result<(), CmdError> { server .wait_for_finish() .await - .map_err(|err| CmdError::Failure(anyhow!("{err}")))?; + .map_err(|err| CmdError::Failure(anyhow!(err)))?; Ok(()) } diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index c615ae3da4..c4687f828c 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -71,7 +71,7 @@ async fn do_run() -> Result<(), CmdError> { match args { Args::Openapi => { - run_openapi().map_err(|err| CmdError::Failure(anyhow!("{err}"))) + run_openapi().map_err(|err| CmdError::Failure(anyhow!(err))) } Args::Run { config_file_path, @@ -134,11 +134,11 @@ async fn do_run() -> Result<(), CmdError> { .map_err(CmdError::Failure)?; let server = Server::start(log, args) .await - .map_err(|err| CmdError::Failure(anyhow!("{err}")))?; + .map_err(|err| CmdError::Failure(anyhow!(err)))?; server .wait_for_finish() .await - .map_err(|err| CmdError::Failure(anyhow!("{err}"))) + .map_err(|err| CmdError::Failure(anyhow!(err))) } } } From ad8e5d601346d61cbae87cf4d8d37c5ce78a6a06 Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Mon, 6 Nov 2023 18:19:49 +0000 Subject: [PATCH 4/8] Prefer anyhow!(err) over err.into() --- gateway/src/bin/mgs.rs | 2 +- nexus/src/bin/nexus.rs | 2 +- sled-agent/src/bin/sled-agent-sim.rs | 12 ++++++------ sled-agent/src/bin/sled-agent.rs | 8 ++++---- sp-sim/src/bin/sp-sim.rs | 8 ++++---- wicketd/src/bin/wicketd.rs | 6 +++--- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs index b83578b384..1ea70c1563 100644 --- a/gateway/src/bin/mgs.rs +++ b/gateway/src/bin/mgs.rs @@ -136,7 +136,7 @@ async fn do_run() -> Result<(), CmdError> { #[cfg(target_os = "illumos")] fn read_smf_config() -> Result { fn scf_to_cmd_err(err: illumos_utils::scf::ScfError) -> CmdError { - CmdError::Failure(err.into()) + CmdError::Failure(anyhow!(err)) } use illumos_utils::scf::ScfHandle; diff --git a/nexus/src/bin/nexus.rs b/nexus/src/bin/nexus.rs index c283af7313..24fef5c8d2 100644 --- a/nexus/src/bin/nexus.rs +++ b/nexus/src/bin/nexus.rs @@ -54,7 +54,7 @@ async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); let config = Config::from_file(args.config_file_path) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; if args.openapi { run_openapi_external().map_err(|err| CmdError::Failure(anyhow!(err))) diff --git a/sled-agent/src/bin/sled-agent-sim.rs b/sled-agent/src/bin/sled-agent-sim.rs index 1ffc9420a8..13146b0060 100644 --- a/sled-agent/src/bin/sled-agent-sim.rs +++ b/sled-agent/src/bin/sled-agent-sim.rs @@ -6,7 +6,7 @@ // TODO see the TODO for nexus. -use anyhow::Context; +use anyhow::{anyhow, Context}; use camino::Utf8PathBuf; use clap::Parser; use dropshot::ConfigDropshot; @@ -95,8 +95,8 @@ async fn main() { async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); - let tmp = - camino_tempfile::tempdir().map_err(|e| CmdError::Failure(e.into()))?; + let tmp = camino_tempfile::tempdir() + .map_err(|e| CmdError::Failure(anyhow!(e)))?; let config = Config { id: args.uuid, sim_mode: args.sim_mode, @@ -125,10 +125,10 @@ async fn do_run() -> Result<(), CmdError> { (Some(cert_path), Some(key_path)) => { let cert_bytes = std::fs::read_to_string(&cert_path) .with_context(|| format!("read {:?}", &cert_path)) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; let key_bytes = std::fs::read_to_string(&key_path) .with_context(|| format!("read {:?}", &key_path)) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; Some(NexusTypes::Certificate { cert: cert_bytes, key: key_bytes }) } _ => { @@ -147,5 +147,5 @@ async fn do_run() -> Result<(), CmdError> { run_standalone_server(&config, &rss_args) .await - .map_err(|e| CmdError::Failure(e.into())) + .map_err(|e| CmdError::Failure(anyhow!(e))) } diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index f4d654c2c1..b8b5abf07f 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -59,7 +59,7 @@ async fn do_run() -> Result<(), CmdError> { }, Args::Run { config_path } => { let config = SledConfig::from_file(&config_path) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; // - Sled agent starts with the normal config file - typically // called "config.toml". @@ -82,7 +82,7 @@ async fn do_run() -> Result<(), CmdError> { let rss_config = if rss_config_path.exists() { Some( RssConfig::from_file(rss_config_path) - .map_err(|e| CmdError::Failure(e.into()))?, + .map_err(|e| CmdError::Failure(anyhow!(e)))?, ) } else { None @@ -90,7 +90,7 @@ async fn do_run() -> Result<(), CmdError> { let server = bootstrap_server::Server::start(config) .await - .map_err(|err| CmdError::Failure(err.into()))?; + .map_err(|err| CmdError::Failure(anyhow!(err)))?; // If requested, automatically supply the RSS configuration. // @@ -102,7 +102,7 @@ async fn do_run() -> Result<(), CmdError> { // abandon the server. Ok(_) | Err(RssAccessError::AlreadyInitialized) => {} Err(e) => { - return Err(CmdError::Failure(e.into())); + return Err(CmdError::Failure(anyhow!(e))); } } } diff --git a/sp-sim/src/bin/sp-sim.rs b/sp-sim/src/bin/sp-sim.rs index 00261cbc3f..85950c9f88 100644 --- a/sp-sim/src/bin/sp-sim.rs +++ b/sp-sim/src/bin/sp-sim.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use anyhow::Result; +use anyhow::{anyhow, Result}; use clap::Parser; use omicron_common::cmd::{fatal, CmdError}; use sp_sim::config::Config; @@ -27,14 +27,14 @@ async fn main() { async fn do_run() -> Result<(), CmdError> { let args = Args::parse(); let config = Config::from_file(args.config_file_path) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; let log = - sp_sim::logger(&config).map_err(|e| CmdError::Failure(e.into()))?; + sp_sim::logger(&config).map_err(|e| CmdError::Failure(anyhow!(e)))?; let _rack = SimRack::start(&config, &log) .await - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; // for now, do nothing except let the spawned tasks run. in the future // (or when used as a library), the expectation is that a caller can diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index c4687f828c..f3ad23f135 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -85,10 +85,10 @@ async fn do_run() -> Result<(), CmdError> { } => { let baseboard = if let Some(baseboard_file) = baseboard_file { let baseboard_file = std::fs::read_to_string(baseboard_file) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; let baseboard: Baseboard = serde_json::from_str(&baseboard_file) - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; // TODO-correctness `Baseboard::unknown()` is slated for removal // after some refactoring in sled-agent, at which point we'll @@ -113,7 +113,7 @@ async fn do_run() -> Result<(), CmdError> { Some(addr) => Some(Ipv6Subnet::new(addr)), None if read_smf_config => { let smf_values = SmfConfigValues::read_current() - .map_err(|e| CmdError::Failure(e.into()))?; + .map_err(|e| CmdError::Failure(anyhow!(e)))?; smf_values.rack_subnet } None => None, From cafa9248365f56ae27a052a76c1eeec0e9f5cac3 Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Mon, 6 Nov 2023 18:23:50 +0000 Subject: [PATCH 5/8] Missed one variant of error.into() --- dev-tools/omicron-dev/src/bin/omicron-dev.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dev-tools/omicron-dev/src/bin/omicron-dev.rs b/dev-tools/omicron-dev/src/bin/omicron-dev.rs index dc0ae05f25..3caa11fdd9 100644 --- a/dev-tools/omicron-dev/src/bin/omicron-dev.rs +++ b/dev-tools/omicron-dev/src/bin/omicron-dev.rs @@ -4,8 +4,7 @@ //! Developer tool for easily running bits of Omicron -use anyhow::bail; -use anyhow::Context; +use anyhow::{anyhow, bail, Context}; use camino::Utf8Path; use camino::Utf8PathBuf; use clap::Args; @@ -34,7 +33,7 @@ async fn main() -> Result<(), anyhow::Error> { OmicronDb::CertCreate { ref args } => cmd_cert_create(args).await, }; if let Err(error) = result { - fatal(CmdError::Failure(error.into())); + fatal(CmdError::Failure(anyhow!(error))); } Ok(()) } From b00b14205a2329c166814da1c5cd544e9ac76ddc Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Mon, 6 Nov 2023 18:25:29 +0000 Subject: [PATCH 6/8] Cleanup string formatting --- gateway/src/bin/mgs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateway/src/bin/mgs.rs b/gateway/src/bin/mgs.rs index 1ea70c1563..6917d4f174 100644 --- a/gateway/src/bin/mgs.rs +++ b/gateway/src/bin/mgs.rs @@ -180,7 +180,7 @@ fn read_smf_config() -> Result { .with_context(|| { format!( "failed to parse `{CONFIG_PG}/{PROP_RACK_ID}` \ - ({prop_rack_id:?}) as a UUID" + ({prop_rack_id:?}) as a UUID" ) }) .map_err(CmdError::Failure)?, From 84537aea59ba3b24adfaf6cd9458356dda6c35e6 Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Mon, 6 Nov 2023 19:16:26 +0000 Subject: [PATCH 7/8] Remove useless conversions --- dev-tools/omicron-dev/src/bin/omicron-dev.rs | 4 ++-- sled-agent/src/bin/sled-agent-sim.rs | 8 +++----- sp-sim/src/bin/sp-sim.rs | 8 +++----- wicketd/src/bin/wicketd.rs | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/dev-tools/omicron-dev/src/bin/omicron-dev.rs b/dev-tools/omicron-dev/src/bin/omicron-dev.rs index 3caa11fdd9..0ab4987f0a 100644 --- a/dev-tools/omicron-dev/src/bin/omicron-dev.rs +++ b/dev-tools/omicron-dev/src/bin/omicron-dev.rs @@ -4,7 +4,7 @@ //! Developer tool for easily running bits of Omicron -use anyhow::{anyhow, bail, Context}; +use anyhow::{bail, Context}; use camino::Utf8Path; use camino::Utf8PathBuf; use clap::Args; @@ -33,7 +33,7 @@ async fn main() -> Result<(), anyhow::Error> { OmicronDb::CertCreate { ref args } => cmd_cert_create(args).await, }; if let Err(error) = result { - fatal(CmdError::Failure(anyhow!(error))); + fatal(CmdError::Failure(error)); } Ok(()) } diff --git a/sled-agent/src/bin/sled-agent-sim.rs b/sled-agent/src/bin/sled-agent-sim.rs index 13146b0060..ee0ebda71e 100644 --- a/sled-agent/src/bin/sled-agent-sim.rs +++ b/sled-agent/src/bin/sled-agent-sim.rs @@ -125,10 +125,10 @@ async fn do_run() -> Result<(), CmdError> { (Some(cert_path), Some(key_path)) => { let cert_bytes = std::fs::read_to_string(&cert_path) .with_context(|| format!("read {:?}", &cert_path)) - .map_err(|e| CmdError::Failure(anyhow!(e)))?; + .map_err(CmdError::Failure)?; let key_bytes = std::fs::read_to_string(&key_path) .with_context(|| format!("read {:?}", &key_path)) - .map_err(|e| CmdError::Failure(anyhow!(e)))?; + .map_err(CmdError::Failure)?; Some(NexusTypes::Certificate { cert: cert_bytes, key: key_bytes }) } _ => { @@ -145,7 +145,5 @@ async fn do_run() -> Result<(), CmdError> { tls_certificate, }; - run_standalone_server(&config, &rss_args) - .await - .map_err(|e| CmdError::Failure(anyhow!(e))) + run_standalone_server(&config, &rss_args).await.map_err(CmdError::Failure) } diff --git a/sp-sim/src/bin/sp-sim.rs b/sp-sim/src/bin/sp-sim.rs index 85950c9f88..ee0dd3b70c 100644 --- a/sp-sim/src/bin/sp-sim.rs +++ b/sp-sim/src/bin/sp-sim.rs @@ -29,12 +29,10 @@ async fn do_run() -> Result<(), CmdError> { let config = Config::from_file(args.config_file_path) .map_err(|e| CmdError::Failure(anyhow!(e)))?; - let log = - sp_sim::logger(&config).map_err(|e| CmdError::Failure(anyhow!(e)))?; + let log = sp_sim::logger(&config).map_err(CmdError::Failure)?; - let _rack = SimRack::start(&config, &log) - .await - .map_err(|e| CmdError::Failure(anyhow!(e)))?; + let _rack = + SimRack::start(&config, &log).await.map_err(CmdError::Failure)?; // for now, do nothing except let the spawned tasks run. in the future // (or when used as a library), the expectation is that a caller can diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index f3ad23f135..887ac496e0 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -113,7 +113,7 @@ async fn do_run() -> Result<(), CmdError> { Some(addr) => Some(Ipv6Subnet::new(addr)), None if read_smf_config => { let smf_values = SmfConfigValues::read_current() - .map_err(|e| CmdError::Failure(anyhow!(e)))?; + .map_err(CmdError::Failure)?; smf_values.rack_subnet } None => None, From 339753b6c821b8c819db09e153a1c7631b1538c7 Mon Sep 17 00:00:00 2001 From: Mike Zeller Date: Tue, 7 Nov 2023 21:31:13 +0000 Subject: [PATCH 8/8] Burned by RUST_BACKTRACE=1 --- nexus/tests/integration_tests/commands.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/nexus/tests/integration_tests/commands.rs b/nexus/tests/integration_tests/commands.rs index 66006e0bdf..02d938b2ac 100644 --- a/nexus/tests/integration_tests/commands.rs +++ b/nexus/tests/integration_tests/commands.rs @@ -56,10 +56,9 @@ fn test_nexus_bad_config() { let (exit_status, stdout_text, stderr_text) = run_command(exec); assert_exit_code(exit_status, EXIT_FAILURE, &stderr_text); assert_contents("tests/output/cmd-nexus-badconfig-stdout", &stdout_text); - assert_eq!( - stderr_text, - format!("nexus: read \"nonexistent\": {}\n", error_for_enoent()) - ); + let expected_err = + format!("nexus: read \"nonexistent\": {}\n", error_for_enoent()); + assert!(&stderr_text.starts_with(&expected_err)); } #[test] @@ -73,13 +72,11 @@ fn test_nexus_invalid_config() { "tests/output/cmd-nexus-invalidconfig-stdout", &stdout_text, ); - assert_eq!( - stderr_text, - format!( - "nexus: parse \"{}\": missing field `deployment`\n", - config_path.display() - ), + let expected_err = format!( + "nexus: parse \"{}\": missing field `deployment`\n", + config_path.display() ); + assert!(&stderr_text.starts_with(&expected_err)); } #[track_caller]