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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 55365CDEA for ; Tue, 11 Jul 2023 14:54:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3E04323678 for ; Tue, 11 Jul 2023 14:54:27 +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, 11 Jul 2023 14:54:26 +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 B282E42E41 for ; Tue, 11 Jul 2023 14:54:25 +0200 (CEST) Date: Tue, 11 Jul 2023 14:54:24 +0200 From: Wolfgang Bumiller To: Max Carrara Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230607163428.1154123-1-m.carrara@proxmox.com> <20230607163428.1154123-2-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230607163428.1154123-2-m.carrara@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.121 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: Re: [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context 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, 11 Jul 2023 12:54:27 -0000 On Wed, Jun 07, 2023 at 06:34:24PM +0200, Max Carrara wrote: > In order to preserve the source(s) of errors, `anyhow::Context` is > used instead of propagating errors via `Result::map_err()` and / or > `anyhow::format_err!()`. > > This makes it possible to access e.g. an underlying `io::Error` or > `nix::Errno` etc. that caused an execution path to fail. > > Certain usages of `anyhow::bail!()` are also changed / replaced > in order to preserve context. > > Signed-off-by: Max Carrara > --- > pbs-client/src/pxar/create.rs | 22 +++-- > pbs-client/src/pxar/dir_stack.rs | 8 +- > pbs-client/src/pxar/extract.rs | 153 ++++++++++++++----------------- > pbs-client/src/pxar/metadata.rs | 44 ++++----- > pbs-client/src/pxar/tools.rs | 13 ++- > 5 files changed, 112 insertions(+), 128 deletions(-) > > diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs > index a9a956c2..2577cf98 100644 > --- a/pbs-client/src/pxar/create.rs > +++ b/pbs-client/src/pxar/create.rs > @@ -7,7 +7,7 @@ use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd}; > use std::path::{Path, PathBuf}; > use std::sync::{Arc, Mutex}; > > -use anyhow::{bail, format_err, Error}; > +use anyhow::{bail, Context, Error}; > use futures::future::BoxFuture; > use futures::FutureExt; > use nix::dir::Dir; > @@ -159,7 +159,7 @@ where > fs_magic, > &mut fs_feature_flags, > ) > - .map_err(|err| format_err!("failed to get metadata for source directory: {}", err))?; > + .context("failed to get metadata for source directory")?; > > let mut device_set = options.device_set.clone(); > if let Some(ref mut set) = device_set { > @@ -441,7 +441,7 @@ impl Archiver { > ) { > Ok(stat) => stat, > Err(ref err) if err.not_found() => continue, > - Err(err) => bail!("stat failed on {:?}: {}", full_path, err), > + Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)), ^ here this is fine > }; > > let match_path = PathBuf::from("/").join(full_path.clone()); > @@ -796,7 +796,7 @@ fn get_fcaps( > Ok(()) > } > Err(Errno::EBADF) => Ok(()), // symlinks > - Err(err) => bail!("failed to read file capabilities: {}", err), > + Err(err) => Err(err).context("failed to read file capabilities"), > } > } > > @@ -818,7 +818,7 @@ fn get_xattr_fcaps_acl( > return Ok(()); > } > Err(Errno::EBADF) => return Ok(()), // symlinks > - Err(err) => bail!("failed to read xattrs: {}", err), > + Err(err) => return Err(err).context("failed to read xattrs"), > }; > > for attr in &xattrs { > @@ -843,7 +843,9 @@ fn get_xattr_fcaps_acl( > Err(Errno::ENODATA) => (), // it got removed while we were iterating... > Err(Errno::EOPNOTSUPP) => (), // shouldn't be possible so just ignore this > Err(Errno::EBADF) => (), // symlinks, shouldn't be able to reach this either > - Err(err) => bail!("error reading extended attribute {:?}: {}", attr, err), > + Err(err) => { > + return Err(err).context(format!("error reading extended attribute {attr:?}")) > + } > } > } > > @@ -858,7 +860,7 @@ fn get_chattr(metadata: &mut Metadata, fd: RawFd) -> Result<(), Error> { > Err(errno) if errno_is_unsupported(errno) => { > return Ok(()); > } > - Err(err) => bail!("failed to read file attributes: {}", err), > + Err(err) => return Err(err).context("failed to read file attributes"), > } > > metadata.stat.flags |= Flags::from_chattr(attr).bits(); > @@ -880,7 +882,7 @@ fn get_fat_attr(metadata: &mut Metadata, fd: RawFd, fs_magic: i64) -> Result<(), > Err(errno) if errno_is_unsupported(errno) => { > return Ok(()); > } > - Err(err) => bail!("failed to read fat attributes: {}", err), > + Err(err) => return Err(err).context("failed to read fat attributes"), > } > > metadata.stat.flags |= Flags::from_fat_attr(attr).bits(); > @@ -919,7 +921,7 @@ fn get_quota_project_id( > if errno_is_unsupported(errno) { > return Ok(()); > } else { > - bail!("error while reading quota project id ({})", errno); > + return Err(errno).context("error while reading quota project id"); > } > } > > @@ -973,7 +975,7 @@ fn get_acl_do( > Err(Errno::EBADF) => return Ok(()), > // Don't bail if there is no data > Err(Errno::ENODATA) => return Ok(()), > - Err(err) => bail!("error while reading ACL - {}", err), > + Err(err) => return Err(err).context("error while reading ACL"), > }; > > process_acl(metadata, acl, acl_type) > diff --git a/pbs-client/src/pxar/dir_stack.rs b/pbs-client/src/pxar/dir_stack.rs > index 0cc4e6a5..43cbee1d 100644 > --- a/pbs-client/src/pxar/dir_stack.rs > +++ b/pbs-client/src/pxar/dir_stack.rs > @@ -2,7 +2,7 @@ use std::ffi::OsString; > use std::os::unix::io::{AsRawFd, BorrowedFd, RawFd}; > use std::path::{Path, PathBuf}; > > -use anyhow::{bail, format_err, Error}; > +use anyhow::{bail, Context, Error}; > use nix::dir::Dir; > use nix::fcntl::OFlag; > use nix::sys::stat::{mkdirat, Mode}; > @@ -130,7 +130,7 @@ impl PxarDirStack { > let dirs_len = self.dirs.len(); > let mut fd = self.dirs[self.created - 1] > .try_as_borrowed_fd() > - .ok_or_else(|| format_err!("lost track of directory file descriptors"))? > + .context("lost track of directory file descriptors")? > .as_raw_fd(); > > while self.created < dirs_len { > @@ -142,7 +142,7 @@ impl PxarDirStack { > > self.dirs[self.created - 1] > .try_as_borrowed_fd() > - .ok_or_else(|| format_err!("lost track of directory file descriptors")) > + .context("lost track of directory file descriptors") > } > > pub fn create_last_dir(&mut self, allow_existing_dirs: bool) -> Result<(), Error> { > @@ -156,7 +156,7 @@ impl PxarDirStack { > > self.dirs[0] > .try_as_borrowed_fd() > - .ok_or_else(|| format_err!("lost track of directory file descriptors")) > + .context("lost track of directory file descriptors") > } > > pub fn path(&self) -> &Path { > diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs > index f6c1991f..7af80bb9 100644 > --- a/pbs-client/src/pxar/extract.rs > +++ b/pbs-client/src/pxar/extract.rs > @@ -8,7 +8,7 @@ use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; > use std::path::{Path, PathBuf}; > use std::sync::{Arc, Mutex}; > > -use anyhow::{bail, format_err, Error}; > +use anyhow::{bail, Context, Error}; > use nix::dir::Dir; > use nix::fcntl::OFlag; > use nix::sys::stat::Mode; > @@ -55,8 +55,8 @@ where > > let root = decoder > .next() > - .ok_or_else(|| format_err!("found empty pxar archive"))? > - .map_err(|err| format_err!("error reading pxar archive: {}", err))?; > + .context("found empty pxar archive")? > + .context("error reading pxar archive")?; > > if !root.is_dir() { > bail!("pxar archive does not start with a directory entry!"); > @@ -67,14 +67,14 @@ where > None, > Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), > ) > - .map_err(|err| format_err!("error creating directory {:?}: {}", destination, err))?; > + .context(format!("error creating directory {:?}", destination))?; ^ but normally, in cases of `map_err` calls, please replace `.context(format!)` with `.with_context(|| format!(...))`, since you're now actually formatting a string which is only *used* in the error case and otherwise actively created and immediately discarded > > let dir = Dir::open( > destination, > OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, > Mode::empty(), > ) > - .map_err(|err| format_err!("unable to open target directory {:?}: {}", destination, err,))?; > + .context(format!("unable to open target directory {:?}", destination))?; ^ same here > > let mut extractor = Extractor::new( > dir, > @@ -92,7 +92,7 @@ where > let mut err_path_stack = vec![OsString::from("/")]; > let mut current_match = options.extract_match_default; > while let Some(entry) = decoder.next() { > - let entry = entry.map_err(|err| format_err!("error reading pxar archive: {}", err))?; > + let entry = entry.context("error reading pxar archive")?; > > let file_name_os = entry.file_name(); > > @@ -102,7 +102,7 @@ where > } > > let file_name = CString::new(file_name_os.as_bytes()) > - .map_err(|_| format_err!("encountered file name with null-bytes"))?; > + .context("encountered file name with null-bytes")?; > > let metadata = entry.metadata(); > > @@ -125,7 +125,7 @@ where > let create = current_match && match_result != Some(MatchType::Exclude); > extractor > .enter_directory(file_name_os.to_owned(), metadata.clone(), create) > - .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?; > + .context(format!("error at entry {:?}", file_name_os))?; ^ same here ... and so forth throughout the patch