[pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Jul 11 14:54:24 CEST 2023
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 <m.carrara at proxmox.com>
> ---
> 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
More information about the pbs-devel
mailing list