From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 18DA894330 for ; Wed, 21 Feb 2024 17:46:33 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DB8951C42D for ; Wed, 21 Feb 2024 17:46:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 21 Feb 2024 17:46:01 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 19A1444571 for ; Wed, 21 Feb 2024 17:46:01 +0100 (CET) Message-ID: <59c1480b-2233-4d1c-a1dd-cea778c0118f@proxmox.com> Date: Wed, 21 Feb 2024 17:45:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pbs-devel@lists.proxmox.com References: <20240220102859.71349-1-g.goller@proxmox.com> <20240220102859.71349-2-g.goller@proxmox.com> From: Max Carrara In-Reply-To: <20240220102859.71349-2-g.goller@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.006 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox-offline-mirror.rs, command.rs, pool.rs, sourceforge.io] Subject: Re: [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Feb 2024 16:46:33 -0000 On 2/20/24 11:28, Gabriel Goller wrote: > From: Fabian Grünbichler > > 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 > Signed-off-by: Gabriel Goller > --- 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, _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); > } > }