From: Max Carrara <m.carrara@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes
Date: Wed, 21 Feb 2024 17:45:59 +0100 [thread overview]
Message-ID: <59c1480b-2233-4d1c-a1dd-cea778c0118f@proxmox.com> (raw)
In-Reply-To: <20240220102859.71349-2-g.goller@proxmox.com>
On 2/20/24 11:28, Gabriel Goller wrote:
> 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>
> ---
There are some lines in `README.rst` that contain only whitespace and some
things regarding ReST formatting. See comments inline.
As these things are non-issues IMO and easily fixed, I'll send in a follow-up fixup
that may be applied and rebased if desired.
> 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 0000000..9c7c213
> --- /dev/null
> +++ b/proxmox-router/README.rst
> @@ -0,0 +1,95 @@
> +================
> + proxmox-router
> +================
AFAIK, our top-level sections consist of the title just being underlined with '='s, as in:
proxmox-router
==============
> +
> +cli
> +==================
This should then instead be underlined with '-' - and the title could be prettier IMO:
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:
In this paragraph (and others) you're using backticks to denote code, but those actually
mark something as "interpreted text" [0]. Instead, double backticks (e.g. ``.with_context``)
are used to mark something as an inline literal [1] (oiw. code).
I know this may be somewhat annoying (as it's not the same as in Markdown), but that's
just how the format works.
[0]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#interpreted-text
[1]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#inline-literals
> +
> +.. 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;
> +
Whitespace in this line here ^
> + -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},
> + };
> +
... and here ^ and also in the remaining inline diff below.
> + -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
Double backticks should be used here as well ...
> +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
... and here.
> +*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 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);
> }
> }
next prev parent reply other threads:[~2024-02-21 16:46 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
2024-02-21 16:45 ` Max Carrara [this message]
2024-02-21 16:52 ` [pbs-devel] [PATCH proxmox] fixup! " Max Carrara
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
2024-04-26 12:00 ` [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-06-17 8:12 ` 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=59c1480b-2233-4d1c-a1dd-cea778c0118f@proxmox.com \
--to=m.carrara@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