Re: [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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@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