From: Gabriel Goller <g.goller@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH proxmox 1/4] CLI: print fatal errors including causes
Date: Fri, 16 Feb 2024 16:33:08 +0100 [thread overview]
Message-ID: <20240216153317.323260-2-g.goller@proxmox.com> (raw)
In-Reply-To: <20240216153317.323260-1-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.
For example, with two calls to `.with_context` when bubbling up errors in
proxmox-offline-mirror:
> ----8<----
> 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(),
> ---->8----
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.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
proxmox-router/src/cli/command.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/proxmox-router/src/cli/command.rs b/proxmox-router/src/cli/command.rs
index 7a26ffb..d5522f1 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
next prev parent reply other threads:[~2024-02-16 15:33 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-02-16 15:33 ` Gabriel Goller [this message]
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
2024-02-16 17:44 ` Max Carrara
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
2024-02-16 17:47 ` Max Carrara
2024-02-19 10:53 ` Gabriel Goller
2024-02-16 17:49 ` [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Max Carrara
2024-02-16 17:55 ` Max Carrara
2024-02-19 10:54 ` Gabriel Goller
2024-02-20 10:30 ` Gabriel Goller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240216153317.323260-2-g.goller@proxmox.com \
--to=g.goller@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox