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 B15A293785 for ; Tue, 20 Feb 2024 11:29:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9863D49E5 for ; Tue, 20 Feb 2024 11:29:04 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 20 Feb 2024 11:29:03 +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 50E1F43DF5 for ; Tue, 20 Feb 2024 11:29:03 +0100 (CET) From: Gabriel Goller To: pbs-devel@lists.proxmox.com Date: Tue, 20 Feb 2024 11:28:50 +0100 Message-ID: <20240220102859.71349-2-g.goller@proxmox.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240220102859.71349-1-g.goller@proxmox.com> References: <20240220102859.71349-1-g.goller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.098 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. [command.rs, proxmox-offline-mirror.rs, pool.rs] Subject: [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: Tue, 20 Feb 2024 10:29:04 -0000 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 --- 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 +================ + +cli +================== +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, _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 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