[pbs-devel] [PATCH proxmox v3 1/4] CLI: print fatal errors including causes

[Gabriel Goller <g.goller@proxmox.com>]

From: Fabian Grünbichler <f.gruenbichler@proxmox.com>

as a first step of improving our error handling story, printing context
and causes if the error contains them.

The downside to adding context is that the default Display implementation
will *just* print the context, which hides the root cause. This is why
we print the errors using the pretty-print formatter in this change.

More info in `proxmox-router/README.rst`.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-router/README.rst         | 95 +++++++++++++++++++++++++++++++
 proxmox-router/src/cli/command.rs |  4 +-
 2 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 proxmox-router/README.rst

diff --git a/proxmox-router/README.rst b/proxmox-router/README.rst
new file mode 100644
index 00000000..5fef633f
--- /dev/null
+++ b/proxmox-router/README.rst
@@ -0,0 +1,95 @@
+proxmox-router
+==============
+
+Command Line
+------------
+
+To improve our error handling story, we use anyhow and ``.context()`` on
+all errors. This means no more ``format_err()`` and ``format()`` of errors.
+
+For example, with two calls to ``.with_context`` when bubbling up errors in
+proxmox-offline-mirror:
+
+.. code-block::
+    diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
+    index bec366a..403a2f5 100644
+    --- a/src/bin/proxmox-offline-mirror.rs
+    +++ b/src/bin/proxmox-offline-mirror.rs
+    @@ -1,7 +1,7 @@
+     use std::fmt::Display;
+     use std::path::Path;
+
+    -use anyhow::{bail, Error};
+    +use anyhow::{bail, format_err, Context, Error};
+     use serde_json::Value;
+
+     use proxmox_router::cli::{run_cli_command, CliCommand, CliCommandMap, CliEnvironment};
+    @@ -676,7 +676,8 @@ async fn setup(config: Option<String>, _param: Value) -> Result<(), Error> {
+                 Action::AddMirror => {
+                     for mirror_config in action_add_mirror(&config)? {
+                         let id = mirror_config.id.clone();
+    -                    mirror::init(&mirror_config)?;
+    +                    mirror::init(&mirror_config)
+    +                        .with_context(|| format!("Failed to initialize mirror '{id}'"))?;
+                         config.set_data(&id, "mirror", mirror_config)?;
+                         save_config(&config_file, &config)?;
+                         println!("Config entry '{id}' added");
+    diff --git a/src/pool.rs b/src/pool.rs
+    index 3da8c08..ecf3f6f 100644
+    --- a/src/pool.rs
+    +++ b/src/pool.rs
+    @@ -7,7 +7,7 @@ use std::{
+         path::{Path, PathBuf},
+     };
+
+    -use anyhow::{bail, format_err, Error};
+    +use anyhow::{bail, format_err, Context, Error};
+     use nix::{unistd, NixPath};
+
+     use proxmox_apt::deb822::CheckSums;
+    @@ -45,10 +45,12 @@ impl Pool {
+             }
+
+             if !pool.exists() {
+    -            create_path(pool, None, None)?;
+    +            create_path(pool, None, None)
+    +                .with_context(|| format!("Failed to create pool dir {pool:?}"))?;
+             }
+
+    -        create_path(link_dir, None, None)?;
+    +        create_path(link_dir, None, None)
+    +            .with_context(|| format!("Failed to create link dir {link_dir:?}"))?;
+
+             Ok(Self {
+                 pool_dir: pool.to_path_buf(),
+
+We'd get the following output::
+
+    Error: Failed to initialize mirror 'debian_bullseye_main'
+
+    Caused by:
+        0: Failed to create pool dir "/var/lib/proxmox-offline-mirror/mirrors//.pool"
+        1: EACCES: Permission denied
+
+Instead of the original::
+
+    Error: EACCESS: Permission denied
+
+Which is not really helpful without knowing the path.
+
+For non-fatal cases or logging inside tasks, ``{:#}`` could be used which just
+prints the causes/contexts in a single line like this::
+
+    Failed to initialize mirror 'debian_bullseye_main': Failed to create pool dir "/var/lib/proxmox-offline-mirror/mirrors//.pool": EACCES: Permission denied
+
+but for that usage, the context should be kept short to avoid the line getting overly long.
+
+One downside to adding context is that the default ``Display`` implementation will
+*just* print the context, which hides the root cause::
+
+    Error: Failed to initialize mirror 'debian_bullseye_main'
+
+When adding context to existing error handling (or when replacing manual
+context adding via format_err!), call sites need to be adapted to ensure the
+causes are not accidentally hidden.
+
diff --git a/proxmox-router/src/cli/command.rs b/proxmox-router/src/cli/command.rs
index 7a26ffb9..d5522f12 100644
--- a/proxmox-router/src/cli/command.rs
+++ b/proxmox-router/src/cli/command.rs
@@ -83,7 +83,7 @@ async fn handle_simple_command_future(
             }
         }
         Err(err) => {
-            eprintln!("Error: {}", err);
+            eprintln!("Error: {:?}", err);
             return Err(err);
         }
     }
@@ -135,7 +135,7 @@ fn handle_simple_command(
             }
         }
         Err(err) => {
-            eprintln!("Error: {}", err);
+            eprintln!("Error: {:?}", err);
             return Err(err);
         }
     }
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel