CmdError error messages should provide more information

oxidecomputer · Nov 3, 2023 · f5a7964 · f5a7964
1 parent dbf01fd
commit f5a7964
Show file tree

Hide file tree

Showing 10 changed files with 118 additions and 99 deletions.
diff --git 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
@@ -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
@@ -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,35 +101,31 @@ 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! {
                     signal = signals.next() => match signal {
                         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
                         // signal.
                         _ => 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<ConfigProperties, CmdError> {
     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,25 +160,31 @@ fn read_smf_config() -> Result<ConfigProperties, CmdError> {
 
     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)?;
 
     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<ConfigProperties, CmdError> {
     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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,15 +82,15 @@ 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
             };
 
             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(())
         }