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

[Max Carrara <m.carrara@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);
>          }
>      }