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 3D5F798BF3 for ; Tue, 18 Apr 2023 10:37:47 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 19165C010 for ; Tue, 18 Apr 2023 10:37:47 +0200 (CEST) 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 ; Tue, 18 Apr 2023 10:37:46 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3140B45762 for ; Tue, 18 Apr 2023 10:37:46 +0200 (CEST) From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= To: pbs-devel@lists.proxmox.com Date: Tue, 18 Apr 2023 10:37:37 +0200 Message-Id: <20230418083737.2428602-1-f.gruenbichler@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.076 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 - Subject: [pbs-devel] [RFC proxmox] 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, 18 Apr 2023 08:37:47 -0000 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, _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 --- RFC despite the off-list echo being positive - ideally we would combine this with using thiserror or something similar to also define nice error types at certain boundaries between our application layer and lower level lib code, which would help with keeping the error chain length manageable.. 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 8ae889f..0d43850 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.30.2