* [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor
@ 2023-06-07 16:34 Max Carrara
2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context Max Carrara
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw)
To: pbs-devel
The goal of this series is to improve how errors occurring during
the extraction of pxar archives are propagated and handled.
This is mainly done through leveraging the `anyhow::Context` trait [0],
which makes it possible to provide and propagate context-dependent error
information (hence its name), while simultaneously preserving the
original error that caused an execution path to fail in the first place.
Furthermore, the existing error handler and its related mechanisms
are now also used during the overall process of extraction.
Together with the introduction of a new type of context variable,
errors that happen during the extraction can now be handled for each
type of entry individually (if desired).
This is demonstrated by adding a new flag to the `proxmox-backup-client`
CLI tool, which appends a handler that ignores errors which occur
during the extraction of a device node. This also fixes #3460. [1]
[0]: https://docs.rs/anyhow/latest/anyhow/trait.Context.html
[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3460
proxmox-backup:
Max Carrara (4):
pbs-client: pxar: preserve error context
pbs-client: pxar: refactor body of `extract_archive` to
`ExtractorIter`
pbs-client: pxar: add `PxarExtractContext`
proxmox-backup-client: restore: add 'ignore-extract-device-errors'
flag
pbs-client/src/pxar/create.rs | 22 +-
pbs-client/src/pxar/dir_stack.rs | 8 +-
pbs-client/src/pxar/extract.rs | 573 +++++++++++++++++++++---------
pbs-client/src/pxar/metadata.rs | 44 +--
pbs-client/src/pxar/mod.rs | 2 +-
pbs-client/src/pxar/tools.rs | 13 +-
proxmox-backup-client/src/main.rs | 61 +++-
7 files changed, 505 insertions(+), 218 deletions(-)
pve-container:
Max Carrara (1):
fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs
src/PVE/LXC/Create.pm | 4 ++++
1 file changed, 4 insertions(+)
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread* [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara @ 2023-06-07 16:34 ` Max Carrara 2023-07-11 12:54 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` Max Carrara ` (3 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw) To: pbs-devel 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)), }; 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))?; 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))?; 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))?; // We're starting a new directory, push our old matching state and replace it with // our new one: @@ -142,16 +142,14 @@ where (_, EntryKind::GoodbyeTable) => { // go up a directory - extractor.set_path(err_path_stack.pop().ok_or_else(|| { - format_err!( - "error at entry {:?}: unexpected end of directory", - file_name_os - ) - })?); + extractor.set_path(err_path_stack.pop().context(format!( + "error at entry {:?} - unexpected end of directory", + file_name_os + ))?); extractor .leave_directory() - .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?; + .context(format!("error at entry {:?}", file_name_os))?; // We left a directory, also get back our previous matching state. This is in sync // with `dir_stack` so this should never be empty except for the final goodbye @@ -196,14 +194,14 @@ where &file_name, metadata, *size, - &mut decoder.contents().ok_or_else(|| { - format_err!("found regular file entry without contents in archive") - })?, + &mut decoder + .contents() + .context("found regular file entry without contents in archive")?, extractor.overwrite, ), (false, _) => Ok(()), // skip this } - .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?; + .context(format!("error at entry {:?}", file_name_os))?; } if !extractor.dir_stack.is_empty() { @@ -254,7 +252,7 @@ impl Extractor { pub fn on_error(&mut self, mut on_error: Box<dyn FnMut(Error) -> Result<(), Error> + Send>) { let path = Arc::clone(&self.current_path); self.on_error = Box::new(move |err: Error| -> Result<(), Error> { - on_error(format_err!("error at {:?}: {}", path.lock().unwrap(), err)) + on_error(err.context(format!("error at {:?}", path.lock().unwrap()))) }); } @@ -291,8 +289,8 @@ impl Extractor { let dir = self .dir_stack .pop() - .map_err(|err| format_err!("unexpected end of directory entry: {}", err))? - .ok_or_else(|| format_err!("broken pxar archive (directory stack underrun)"))?; + .context("unexpected end of directory entry")? + .context("broken pxar archive (directory stack underrun)")?; if let Some(fd) = dir.try_as_borrowed_fd() { metadata::apply( @@ -302,7 +300,7 @@ impl Extractor { &path_info, &mut self.on_error, ) - .map_err(|err| format_err!("failed to apply directory metadata: {}", err))?; + .context("failed to apply directory metadata")?; } Ok(()) @@ -316,7 +314,7 @@ impl Extractor { self.dir_stack .last_dir_fd(self.allow_existing_dirs) .map(|d| d.as_raw_fd()) - .map_err(|err| format_err!("failed to get parent directory file descriptor: {}", err)) + .context("failed to get parent directory file descriptor") } pub fn extract_symlink( @@ -370,16 +368,12 @@ impl Extractor { device: libc::dev_t, ) -> Result<(), Error> { let mode = metadata.stat.mode; - let mode = u32::try_from(mode).map_err(|_| { - format_err!( - "device node's mode contains illegal bits: 0x{:x} (0o{:o})", - mode, - mode, - ) - })?; + let mode = u32::try_from(mode).context(format!( + "device node's mode contains illegal bits: 0x{mode:x} (0o{mode:o})" + ))?; let parent = self.parent_fd()?; unsafe { c_result!(libc::mknodat(parent, file_name.as_ptr(), mode, device)) } - .map_err(|err| format_err!("failed to create device node: {}", err))?; + .context("failed to create device node")?; metadata::apply_at( self.feature_flags, @@ -409,7 +403,7 @@ impl Extractor { let mut file = unsafe { std::fs::File::from_raw_fd( nix::fcntl::openat(parent, file_name, oflags, Mode::from_bits(0o600).unwrap()) - .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?, + .context(format!("failed to create file {file_name:?}"))?, ) }; @@ -419,10 +413,10 @@ impl Extractor { file.as_raw_fd(), &mut self.on_error, ) - .map_err(|err| format_err!("failed to apply initial flags: {}", err))?; + .context("failed to apply initial flags")?; - let result = sparse_copy(&mut *contents, &mut file) - .map_err(|err| format_err!("failed to copy file contents: {}", err))?; + let result = + sparse_copy(&mut *contents, &mut file).context("failed to copy file contents")?; if size != result.written { bail!( @@ -436,7 +430,7 @@ impl Extractor { while match nix::unistd::ftruncate(file.as_raw_fd(), size as i64) { Ok(_) => false, Err(errno) if errno == nix::errno::Errno::EINTR => true, - Err(err) => bail!("error setting file size: {}", err), + Err(err) => return Err(err).context("error setting file size"), } {} } @@ -467,7 +461,7 @@ impl Extractor { let mut file = tokio::fs::File::from_std(unsafe { std::fs::File::from_raw_fd( nix::fcntl::openat(parent, file_name, oflags, Mode::from_bits(0o600).unwrap()) - .map_err(|err| format_err!("failed to create file {:?}: {}", file_name, err))?, + .context(format!("failed to create file {file_name:?}"))?, ) }); @@ -477,11 +471,11 @@ impl Extractor { file.as_raw_fd(), &mut self.on_error, ) - .map_err(|err| format_err!("failed to apply initial flags: {}", err))?; + .context("failed to apply initial flags")?; let result = sparse_copy_async(&mut *contents, &mut file) .await - .map_err(|err| format_err!("failed to copy file contents: {}", err))?; + .context("failed to copy file contents")?; if size != result.written { bail!( @@ -495,7 +489,7 @@ impl Extractor { while match nix::unistd::ftruncate(file.as_raw_fd(), size as i64) { Ok(_) => false, Err(errno) if errno == nix::errno::Errno::EINTR => true, - Err(err) => bail!("error setting file size: {}", err), + Err(err) => return Err(err).context("error setting file size"), } {} } @@ -532,12 +526,12 @@ where header.set_size(size); add_metadata_to_header(&mut header, metadata); header.set_cksum(); + match contents { Some(content) => tar.add_entry(&mut header, path, content).await, None => tar.add_entry(&mut header, path, tokio::io::empty()).await, } - .map_err(|err| format_err!("could not send file entry: {}", err))?; - Ok(()) + .context("could not send file entry") } /// Creates a tar file from `path` and writes it into `output` @@ -551,7 +545,7 @@ where let file = root .lookup(&path) .await? - .ok_or_else(|| format_err!("error opening '{:?}'", path.as_ref()))?; + .context(format!("error opening {:?}", path.as_ref()))?; let mut components = file.entry().path().components(); components.next_back(); // discard last @@ -574,13 +568,13 @@ where tarencoder .add_entry(&mut header, path, tokio::io::empty()) .await - .map_err(|err| format_err!("could not send dir entry: {}", err))?; + .context("could not send dir entry")?; } let mut decoder = dir.decode_full().await?; decoder.enable_goodbye_entries(false); while let Some(entry) = decoder.next().await { - let entry = entry.map_err(|err| format_err!("cannot decode entry: {}", err))?; + let entry = entry.context("cannot decode entry")?; let metadata = entry.metadata(); let path = entry.path().strip_prefix(prefix)?; @@ -595,7 +589,7 @@ where let entry = root .lookup(&path) .await? - .ok_or_else(|| format_err!("error looking up '{:?}'", path))?; + .context(format!("error looking up {path:?}"))?; let realfile = accessor.follow_hardlink(&entry).await?; let metadata = realfile.entry().metadata(); let realpath = Path::new(link); @@ -630,7 +624,7 @@ where tarencoder .add_link(&mut header, path, stripped_path) .await - .map_err(|err| format_err!("could not send hardlink entry: {}", err))?; + .context("could not send hardlink entry")?; } } EntryKind::Symlink(link) if !link.data.is_empty() => { @@ -643,7 +637,7 @@ where tarencoder .add_link(&mut header, path, realpath) .await - .map_err(|err| format_err!("could not send symlink entry: {}", err))?; + .context("could not send symlink entry")?; } EntryKind::Fifo => { log::debug!("adding '{}' to tar", path.display()); @@ -657,7 +651,7 @@ where tarencoder .add_entry(&mut header, path, tokio::io::empty()) .await - .map_err(|err| format_err!("could not send fifo entry: {}", err))?; + .context("coult not send fifo entry")?; } EntryKind::Directory => { log::debug!("adding '{}' to tar", path.display()); @@ -671,7 +665,7 @@ where tarencoder .add_entry(&mut header, path, tokio::io::empty()) .await - .map_err(|err| format_err!("could not send dir entry: {}", err))?; + .context("could not send dir entry")?; } } EntryKind::Device(device) => { @@ -690,7 +684,7 @@ where tarencoder .add_entry(&mut header, path, tokio::io::empty()) .await - .map_err(|err| format_err!("could not send device entry: {}", err))?; + .context("could not send device entry")?; } _ => {} // ignore all else } @@ -714,7 +708,7 @@ where let file = root .lookup(&path) .await? - .ok_or_else(|| format_err!("error opening '{:?}'", path.as_ref()))?; + .context(format!("error opening {:?}", path.as_ref()))?; let prefix = { let mut components = file.entry().path().components(); @@ -756,13 +750,13 @@ where ); zip.add_entry(entry, decoder.contents()) .await - .map_err(|err| format_err!("could not send file entry: {}", err))?; + .context("could not send file entry")?; } EntryKind::Hardlink(_) => { let entry = root .lookup(&path) .await? - .ok_or_else(|| format_err!("error looking up '{:?}'", path))?; + .context(format!("error looking up {:?}", path))?; let realfile = accessor.follow_hardlink(&entry).await?; let metadata = realfile.entry().metadata(); log::debug!("adding '{}' to zip", path.display()); @@ -774,7 +768,7 @@ where ); zip.add_entry(entry, decoder.contents()) .await - .map_err(|err| format_err!("could not send file entry: {}", err))?; + .context("could not send file entry")?; } EntryKind::Directory => { log::debug!("adding '{}' to zip", path.display()); @@ -806,26 +800,20 @@ where None, Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), ) - .map_err(|err| { - format_err!( - "error creating directory {:?}: {}", - destination.as_ref(), - err - ) - })?; + .context(format!( + "error creating directory {:?}", + destination.as_ref() + ))?; let dir = Dir::open( destination.as_ref(), OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, Mode::empty(), ) - .map_err(|err| { - format_err!( - "unable to open target directory {:?}: {}", - destination.as_ref(), - err, - ) - })?; + .context(format!( + "unable to open target directory {:?}", + destination.as_ref() + ))?; Ok(Extractor::new(dir, metadata, false, false, Flags::DEFAULT)) } @@ -850,7 +838,7 @@ where let file = root .lookup(&path) .await? - .ok_or_else(|| format_err!("error opening '{:?}'", path.as_ref()))?; + .context(format!("error opening {:?}", path.as_ref()))?; recurse_files_extractor(&mut extractor, file).await } @@ -866,7 +854,7 @@ where decoder.enable_goodbye_entries(true); let root = match decoder.next().await { Some(Ok(root)) => root, - Some(Err(err)) => bail!("error getting root entry from pxar: {}", err), + Some(Err(err)) => return Err(err).context("error getting root entry from pxar"), None => bail!("cannot extract empty archive"), }; @@ -920,8 +908,8 @@ fn get_filename(entry: &Entry) -> Result<(OsString, CString), Error> { bail!("archive file entry contains slashes, which is invalid and a security concern"); } - let file_name = CString::new(file_name_os.as_bytes()) - .map_err(|_| format_err!("encountered file name with null-bytes"))?; + let file_name = + CString::new(file_name_os.as_bytes()).context("encountered file name with null-bytes")?; Ok((file_name_os, file_name)) } @@ -943,7 +931,7 @@ where EntryKind::Directory => { extractor .enter_directory(file_name_os.to_owned(), metadata.clone(), true) - .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?; + .context(format!("error at entry {file_name_os:?}"))?; let dir = file.enter_directory().await?; let mut seq_decoder = dir.decode_full().await?; @@ -957,9 +945,10 @@ where &file_name, metadata, *size, - &mut file.contents().await.map_err(|_| { - format_err!("found regular file entry without contents in archive") - })?, + &mut file + .contents() + .await + .context("found regular file entry without contents in archive")?, extractor.overwrite, ) .await? @@ -997,7 +986,7 @@ where dir_level += 1; extractor .enter_directory(file_name_os.to_owned(), metadata.clone(), true) - .map_err(|err| format_err!("error at entry {:?}: {}", file_name_os, err))?; + .context(format!("error at entry {file_name_os:?}"))?; } EntryKind::File { size, .. } => { extractor @@ -1005,9 +994,9 @@ where &file_name, metadata, *size, - &mut decoder.contents().ok_or_else(|| { - format_err!("found regular file entry without contents in archive") - })?, + &mut decoder + .contents() + .context("found regular file entry without contents in archive")?, extractor.overwrite, ) .await? diff --git a/pbs-client/src/pxar/metadata.rs b/pbs-client/src/pxar/metadata.rs index cff3cb34..745785bf 100644 --- a/pbs-client/src/pxar/metadata.rs +++ b/pbs-client/src/pxar/metadata.rs @@ -2,7 +2,7 @@ use std::ffi::{CStr, CString}; use std::os::unix::io::{AsRawFd, RawFd}; use std::path::Path; -use anyhow::{bail, format_err, Error}; +use anyhow::{anyhow, bail, Context, Error}; use nix::errno::Errno; use nix::fcntl::OFlag; use nix::sys::stat::Mode; @@ -106,7 +106,7 @@ pub fn apply( .or_else(&mut *on_error)?; add_fcaps(flags, c_proc_path.as_ptr(), metadata, &mut skip_xattrs).or_else(&mut *on_error)?; apply_acls(flags, &c_proc_path, metadata, path_info) - .map_err(|err| format_err!("failed to apply acls: {}", err)) + .context("failed to apply acls") .or_else(&mut *on_error)?; apply_quota_project_id(flags, fd, metadata).or_else(&mut *on_error)?; @@ -118,7 +118,7 @@ pub fn apply( }) .map(drop) .or_else(allow_notsupp) - .map_err(|err| format_err!("failed to change file mode: {}", err)) + .context("failed to change file mode") .or_else(&mut *on_error)?; } @@ -134,11 +134,9 @@ pub fn apply( Ok(_) => (), Err(ref err) if err.is_errno(Errno::EOPNOTSUPP) => (), Err(err) => { - on_error(format_err!( - "failed to restore mtime attribute on {:?}: {}", - path_info, - err - ))?; + on_error(anyhow!(err).context(format!( + "failed to restore mtime attribute on {path_info:?}" + )))?; } } @@ -167,7 +165,7 @@ pub fn apply_ownership( )) .map(drop) .or_else(allow_notsupp) - .map_err(|err| format_err!("failed to set ownership: {}", err)) + .context("failed to set ownership") .or_else(&mut *on_error)?; } Ok(()) @@ -198,9 +196,7 @@ fn add_fcaps( }) .map(drop) .or_else(|err| allow_notsupp_remember(err, skip_xattrs)) - .map_err(|err| format_err!("failed to apply file capabilities: {}", err))?; - - Ok(()) + .context("failed to apply file capabilities") } fn apply_xattrs( @@ -234,7 +230,7 @@ fn apply_xattrs( }) .map(drop) .or_else(|err| allow_notsupp_remember(err, &mut *skip_xattrs)) - .map_err(|err| format_err!("failed to apply extended attributes: {}", err))?; + .context("failed to apply extended attributes")?; } Ok(()) @@ -348,21 +344,13 @@ fn apply_quota_project_id(flags: Flags, fd: RawFd, metadata: &Metadata) -> Resul let mut fsxattr = fs::FSXAttr::default(); unsafe { - fs::fs_ioc_fsgetxattr(fd, &mut fsxattr).map_err(|err| { - format_err!( - "error while getting fsxattr to restore quota project id - {}", - err - ) - })?; + fs::fs_ioc_fsgetxattr(fd, &mut fsxattr) + .context("error while getting fsxattr to restore quota project id")?; fsxattr.fsx_projid = projid.projid as u32; - fs::fs_ioc_fssetxattr(fd, &fsxattr).map_err(|err| { - format_err!( - "error while setting fsxattr to restore quota project id - {}", - err - ) - })?; + fs::fs_ioc_fssetxattr(fd, &fsxattr) + .context("error while setting fsxattr to restore quota project id")?; } Ok(()) @@ -386,7 +374,7 @@ fn apply_chattr(fd: RawFd, chattr: libc::c_long, mask: libc::c_long) -> Result<( 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"), } let attr = (chattr & mask) | (fattr & !mask); @@ -398,7 +386,7 @@ fn apply_chattr(fd: RawFd, chattr: libc::c_long, mask: libc::c_long) -> Result<( match unsafe { fs::write_attr_fd(fd, &attr) } { Ok(_) => Ok(()), Err(errno) if errno_is_unsupported(errno) => Ok(()), - Err(err) => bail!("failed to set file attributes: {}", err), + Err(err) => Err(err).context("failed to set file attributes"), } } @@ -412,7 +400,7 @@ fn apply_flags(flags: Flags, fd: RawFd, entry_flags: u64) -> Result<(), Error> { match unsafe { fs::write_fat_attr_fd(fd, &fatattr) } { Ok(_) => (), Err(errno) if errno_is_unsupported(errno) => (), - Err(err) => bail!("failed to set file FAT attributes: {}", err), + Err(err) => return Err(err).context("failed to set file FAT attributes"), } } diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs index 844a0f73..271df5d3 100644 --- a/pbs-client/src/pxar/tools.rs +++ b/pbs-client/src/pxar/tools.rs @@ -4,7 +4,7 @@ use std::ffi::OsStr; use std::os::unix::ffi::OsStrExt; use std::path::Path; -use anyhow::{bail, format_err, Error}; +use anyhow::{bail, Context, Error}; use nix::sys::stat::Mode; use pxar::{format::StatxTimestamp, mode, Entry, EntryKind, Metadata}; @@ -12,10 +12,15 @@ use pxar::{format::StatxTimestamp, mode, Entry, EntryKind, Metadata}; /// Get the file permissions as `nix::Mode` pub fn perms_from_metadata(meta: &Metadata) -> Result<Mode, Error> { let mode = meta.stat.get_permission_bits(); + u32::try_from(mode) - .map_err(drop) - .and_then(|mode| Mode::from_bits(mode).ok_or(())) - .map_err(|_| format_err!("mode contains illegal bits: 0x{:x} (0o{:o})", mode, mode)) + .context("couldn't narrow permission bits") + .and_then(|mode| { + Mode::from_bits(mode).context(format!( + "mode contains illegal bits: 0x{:x} (0o{:o})", + mode, mode + )) + }) } /// Make sure path is relative and not '.' or '..'. -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context Max Carrara @ 2023-07-11 12:54 ` Wolfgang Bumiller 0 siblings, 0 replies; 10+ messages in thread From: Wolfgang Bumiller @ 2023-07-11 12:54 UTC (permalink / raw) To: Max Carrara; +Cc: pbs-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context Max Carrara @ 2023-06-07 16:34 ` Max Carrara 2023-07-11 13:24 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 3/5] pbs-client: pxar: add `PxarExtractContext` Max Carrara ` (2 subsequent siblings) 4 siblings, 1 reply; 10+ messages in thread From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw) To: pbs-devel This change factors the body of `extract_archive()` into a separate struct named `ExtractorIter` which implements the `Iterator` trait. This refactor has two goals: * Make it easier to provide and propagate errors and additional information via `anyhow::Context` * Introduce a means to handle errors that occur during extraction, with the possibility to continue extraction if the handler decides that the error is not fatal The latter point benefits from the information provided by the former; previously, errors could only be handled in certain locations (e.g. application of metadata), but not on a "per-entry" basis. Since `extract_archive()` was already using a "desugared" version of the iterator pattern to begin with, wrapping its body up in an actual `Iterator` made the most sense, as it didn't require changing the already existing control flow that much. Signed-off-by: Max Carrara <m.carrara@proxmox.com> --- pbs-client/src/pxar/extract.rs | 382 +++++++++++++++++++++++---------- 1 file changed, 273 insertions(+), 109 deletions(-) diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs index 7af80bb9..fa08bfd7 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, Context, Error}; +use anyhow::{anyhow, bail, Context, Error}; use nix::dir::Dir; use nix::fcntl::OFlag; use nix::sys::stat::Mode; @@ -40,75 +40,224 @@ pub struct PxarExtractOptions<'a> { pub type ErrorHandler = Box<dyn FnMut(Error) -> Result<(), Error> + Send>; pub fn extract_archive<T, F>( - mut decoder: pxar::decoder::Decoder<T>, + decoder: pxar::decoder::Decoder<T>, destination: &Path, feature_flags: Flags, - mut callback: F, + callback: F, options: PxarExtractOptions, ) -> Result<(), Error> where T: pxar::decoder::SeqRead, F: FnMut(&Path), { - // we use this to keep track of our directory-traversal - decoder.enable_goodbye_entries(true); + ExtractorIter::new(decoder, destination, feature_flags, callback, options) + .context("failed to initialize extractor")? + .collect::<Result<(), Error>>() + .context("encountered unexpected error during extraction") +} - let root = decoder - .next() - .context("found empty pxar archive")? - .context("error reading pxar archive")?; +struct ExtractorIterState { + match_stack: Vec<bool>, + err_path_stack: Vec<OsString>, + current_match: bool, + end_reached: bool, +} - if !root.is_dir() { - bail!("pxar archive does not start with a directory entry!"); +/// An [`Iterator`] that encapsulates the process of extraction in [extract_archive]. +/// Therefore, traversing over an [`ExtractorIter`] until exhaustion extracts an +/// entire PXAR archive. +struct ExtractorIter<'a, T, F> +where + T: pxar::decoder::SeqRead, + F: FnMut(&Path), +{ + decoder: pxar::decoder::Decoder<T>, + callback: F, + extractor: Extractor, + match_list: &'a [MatchEntry], + state: ExtractorIterState, +} + +impl ExtractorIterState { + fn new(options: &PxarExtractOptions) -> Self { + Self { + match_stack: Vec::new(), + err_path_stack: Vec::new(), + current_match: options.extract_match_default, + end_reached: false, + } } +} - create_path( - destination, - None, - Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), - ) - .context(format!("error creating directory {:?}", destination))?; +impl<'a, T, F> ExtractorIter<'a, T, F> +where + T: pxar::decoder::SeqRead, + F: FnMut(&Path), +{ + /// Creates and initializes the state of a new [`ExtractorIter`]. + /// + /// This function requires that the given [`Decoder`][D] has not made a single + /// traversal (a call to [`next()`][next]) yet. + /// + /// [D]: pxar::decoder::Decoder + /// [next]: std::iter::Iterator::next() + fn new( + mut decoder: pxar::decoder::Decoder<T>, + destination: &Path, + feature_flags: Flags, + callback: F, + options: PxarExtractOptions<'a>, + ) -> Result<Self, Error> { + // we use this to keep track of our directory-traversal + decoder.enable_goodbye_entries(true); + + let root = decoder + .next() + .context("found empty pxar archive")? + .context("error reading pxar archive")?; + + if !root.is_dir() { + bail!("pxar archive does not start with a directory entry!"); + } - let dir = Dir::open( - destination, - OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, - Mode::empty(), - ) - .context(format!("unable to open target directory {:?}", destination))?; - - let mut extractor = Extractor::new( - dir, - root.metadata().clone(), - options.allow_existing_dirs, - options.overwrite, - feature_flags, - ); - - if let Some(on_error) = options.on_error { - extractor.on_error(on_error); + let mut state = ExtractorIterState::new(&options); + state.err_path_stack.push(OsString::from("/")); + + create_path( + destination, + None, + Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), + ) + .context(format!("error creating directory {destination:?}"))?; + + let dir = Dir::open( + destination, + OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, + Mode::empty(), + ) + .context(format!("unable to open target directory {destination:?}"))?; + + let mut extractor = Extractor::new( + dir, + root.metadata().clone(), + options.allow_existing_dirs, + options.overwrite, + feature_flags, + ); + + if let Some(on_error) = options.on_error { + extractor.on_error(on_error); + } + + Ok(Self { + decoder, + callback, + extractor, + match_list: options.match_list, + state, + }) } - let mut match_stack = Vec::new(); - 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.context("error reading pxar archive")?; + #[inline(always)] + fn callback(&mut self, path: &Path) { + (self.callback)(path) + } - let file_name_os = entry.file_name(); + /// Yields the [`Decoder`][D]'s [`Result`]s and validates each [`Entry`][E] + /// it encounters. It also controls the [`ExtractorIter`]'s stop condition. + /// + /// In detail, the [`ExtractorIter`] will stop if and only if one of the + /// following conditions is true: + /// * The [`Decoder`][D] is exhausted + /// * The [`Decoder`][D] failed to read from the archive and consequently + /// yielded an [`io::Error`] + /// * Validation of an [`Entry`][E] failed + /// + /// The iterator will yield the encountered error before stopping. + /// + /// [D]: pxar::decoder::Decoder + /// [E]: pxar::Entry + #[inline] + fn next_entry(&mut self) -> Option<Result<Entry, Error>> { + if self.state.end_reached { + return None; + } - // safety check: a file entry in an archive must never contain slashes: - if file_name_os.as_bytes().contains(&b'/') { - bail!("archive file entry contains slashes, which is invalid and a security concern"); + let entry = self.decoder.next(); + + if entry.is_none() { + self.state.end_reached = true; + if !self.extractor.dir_stack.is_empty() { + return Some(Err(anyhow!("unexpected eof while decoding pxar archive"))); + } else { + return None; + } } - let file_name = CString::new(file_name_os.as_bytes()) - .context("encountered file name with null-bytes")?; + #[inline(always)] + fn validate_entry(entry: Result<Entry, io::Error>) -> Result<Entry, Error> { + let entry = entry.context("error reading pxar archive")?; + + let file_name_bytes = entry.file_name().as_bytes(); + + if file_name_bytes.contains(&b'/') { + bail!( + "archive file entry contains slashes, which is invalid and a security concern" + ); + } + + CString::new(file_name_bytes).context("encountered file name with null-bytes")?; + + Ok(entry) + } + + let entry = validate_entry(entry.unwrap()); + + if entry.is_err() { + self.state.end_reached = true; + } + Some(entry) + } +} + +impl<'a, T, F> Iterator for ExtractorIter<'a, T, F> +where + T: pxar::decoder::SeqRead, + F: FnMut(&Path), +{ + type Item = Result<(), Error>; + + /// Performs the actual extraction of decoded [`Entries`][E]. + /// + /// Should an error occur during any point of extraction (**not** while + /// fetching the next [`Entry`][E]), the error may be handled by the + /// [`ErrorHandler`] provided by the [`PxarExtractOptions`] used to + /// initialize the iterator. + /// + /// [E]: pxar::Entry + fn next(&mut self) -> Option<Self::Item> { + let entry = self.next_entry()?; + + if entry.is_err() { + assert!( + self.state.end_reached, + "encountered an error while reading the archive, but the iterator was not stopped!" + ); + + return Some(entry.map(drop)); + } + + let entry = entry.unwrap(); + let file_name_os = entry.file_name(); let metadata = entry.metadata(); - extractor.set_path(entry.path().as_os_str().to_owned()); + // can unwrap here because next_entry already checked for null bytes + let file_name = CString::new(file_name_os.as_bytes()).unwrap(); + + self.extractor.set_path(entry.path().as_os_str().to_owned()); - let match_result = options.match_list.matches( + let match_result = self.match_list.matches( entry.path().as_os_str().as_bytes(), Some(metadata.file_type() as u32), ); @@ -116,99 +265,114 @@ where let did_match = match match_result { Some(MatchType::Include) => true, Some(MatchType::Exclude) => false, - None => current_match, + None => self.state.current_match, }; - match (did_match, entry.kind()) { - (_, EntryKind::Directory) => { - callback(entry.path()); - - let create = current_match && match_result != Some(MatchType::Exclude); - extractor - .enter_directory(file_name_os.to_owned(), metadata.clone(), create) - .context(format!("error at entry {:?}", file_name_os))?; - // We're starting a new directory, push our old matching state and replace it with - // our new one: - match_stack.push(current_match); - current_match = did_match; - - // When we hit the goodbye table we'll try to apply metadata to the directory, but - // the Goodbye entry will not contain the path, so push it to our path stack for - // error messages: - err_path_stack.push(extractor.clone_path()); + let extract_res = match (did_match, entry.kind()) { + (_, EntryKind::Directory) => { + self.callback(entry.path()); + + let create = self.state.current_match && match_result != Some(MatchType::Exclude); + let res = self.extractor.enter_directory( + file_name_os.to_owned(), + metadata.clone(), + create, + ); + + if res.is_ok() { + // We're starting a new directory, push our old matching state and replace it with + // our new one: + self.state.match_stack.push(self.state.current_match); + self.state.current_match = did_match; + + // When we hit the goodbye table we'll try to apply metadata to the directory, but + // the Goodbye entry will not contain the path, so push it to our path stack for + // error messages: + self.state.err_path_stack.push(self.extractor.clone_path()); + } - Ok(()) + res } (_, EntryKind::GoodbyeTable) => { // go up a directory - extractor.set_path(err_path_stack.pop().context(format!( - "error at entry {:?} - unexpected end of directory", - file_name_os - ))?); - - extractor - .leave_directory() - .context(format!("error at entry {:?}", file_name_os))?; - - // We left a directory, also get back our previous matching state. This is in sync - // with `dir_stack` so this should never be empty except for the final goodbye - // table, in which case we get back to the default of `true`. - current_match = match_stack.pop().unwrap_or(true); + let res = self + .state + .err_path_stack + .pop() + .context("unexpected end of directory") + .map(|path| self.extractor.set_path(path)) + .and(self.extractor.leave_directory()); + + if res.is_ok() { + // We left a directory, also get back our previous matching state. This is in sync + // with `dir_stack` so this should never be empty except for the final goodbye + // table, in which case we get back to the default of `true`. + self.state.current_match = self.state.match_stack.pop().unwrap_or(true); + } - Ok(()) + res } (true, EntryKind::Symlink(link)) => { - callback(entry.path()); - extractor.extract_symlink(&file_name, metadata, link.as_ref()) + self.callback(entry.path()); + self.extractor + .extract_symlink(&file_name, metadata, link.as_ref()) } (true, EntryKind::Hardlink(link)) => { - callback(entry.path()); - extractor.extract_hardlink(&file_name, link.as_os_str()) + self.callback(entry.path()); + self.extractor + .extract_hardlink(&file_name, link.as_os_str()) } (true, EntryKind::Device(dev)) => { - if extractor.contains_flags(Flags::WITH_DEVICE_NODES) { - callback(entry.path()); - extractor.extract_device(&file_name, metadata, dev) + if self.extractor.contains_flags(Flags::WITH_DEVICE_NODES) { + self.callback(entry.path()); + self.extractor.extract_device(&file_name, metadata, dev) } else { Ok(()) } } (true, EntryKind::Fifo) => { - if extractor.contains_flags(Flags::WITH_FIFOS) { - callback(entry.path()); - extractor.extract_special(&file_name, metadata, 0) + if self.extractor.contains_flags(Flags::WITH_FIFOS) { + self.callback(entry.path()); + self.extractor.extract_special(&file_name, metadata, 0) } else { Ok(()) } } (true, EntryKind::Socket) => { - if extractor.contains_flags(Flags::WITH_SOCKETS) { - callback(entry.path()); - extractor.extract_special(&file_name, metadata, 0) + if self.extractor.contains_flags(Flags::WITH_SOCKETS) { + self.callback(entry.path()); + self.extractor.extract_special(&file_name, metadata, 0) } else { Ok(()) } } - (true, EntryKind::File { size, .. }) => extractor.extract_file( - &file_name, - metadata, - *size, - &mut decoder - .contents() - .context("found regular file entry without contents in archive")?, - extractor.overwrite, - ), + (true, EntryKind::File { size, .. }) => { + let contents = self.decoder.contents(); + + if let Some(mut contents) = contents { + self.extractor.extract_file( + &file_name, + metadata, + *size, + &mut contents, + self.extractor.overwrite, + ) + } else { + Err(anyhow!( + "found regular file entry without contents in archive" + )) + } + } (false, _) => Ok(()), // skip this - } - .context(format!("error at entry {:?}", file_name_os))?; - } + }; - if !extractor.dir_stack.is_empty() { - bail!("unexpected eof while decoding pxar archive"); + Some( + extract_res + .context(format!("error at entry {file_name_os:?}")) + .or_else(&mut *self.extractor.on_error), + ) } - - Ok(()) } /// Common state for file extraction. -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` Max Carrara @ 2023-07-11 13:24 ` Wolfgang Bumiller 0 siblings, 0 replies; 10+ messages in thread From: Wolfgang Bumiller @ 2023-07-11 13:24 UTC (permalink / raw) To: Max Carrara; +Cc: pbs-devel On Wed, Jun 07, 2023 at 06:34:25PM +0200, Max Carrara wrote: > This change factors the body of `extract_archive()` into a separate > struct named `ExtractorIter` which implements the `Iterator` trait. > > This refactor has two goals: > * Make it easier to provide and propagate errors and additional > information via `anyhow::Context` > * Introduce a means to handle errors that occur during extraction, > with the possibility to continue extraction if the handler decides > that the error is not fatal Does it really help much with that both those things? Which parts actually benefit from this? > > The latter point benefits from the information provided by the former; > previously, errors could only be handled in certain locations > (e.g. application of metadata), but not on a "per-entry" basis. > > Since `extract_archive()` was already using a "desugared" version of > the iterator pattern to begin with, wrapping its body up in an actual > `Iterator` made the most sense, as it didn't require changing the already > existing control flow that much. > > Signed-off-by: Max Carrara <m.carrara@proxmox.com> > --- > pbs-client/src/pxar/extract.rs | 382 +++++++++++++++++++++++---------- > 1 file changed, 273 insertions(+), 109 deletions(-) > > diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs > index 7af80bb9..fa08bfd7 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, Context, Error}; > +use anyhow::{anyhow, bail, Context, Error}; > use nix::dir::Dir; > use nix::fcntl::OFlag; > use nix::sys::stat::Mode; > @@ -40,75 +40,224 @@ pub struct PxarExtractOptions<'a> { > pub type ErrorHandler = Box<dyn FnMut(Error) -> Result<(), Error> + Send>; > > pub fn extract_archive<T, F>( > - mut decoder: pxar::decoder::Decoder<T>, > + decoder: pxar::decoder::Decoder<T>, > destination: &Path, > feature_flags: Flags, > - mut callback: F, > + callback: F, > options: PxarExtractOptions, > ) -> Result<(), Error> > where > T: pxar::decoder::SeqRead, > F: FnMut(&Path), > { > - // we use this to keep track of our directory-traversal > - decoder.enable_goodbye_entries(true); > + ExtractorIter::new(decoder, destination, feature_flags, callback, options) > + .context("failed to initialize extractor")? > + .collect::<Result<(), Error>>() > + .context("encountered unexpected error during extraction") > +} > > - let root = decoder > - .next() > - .context("found empty pxar archive")? > - .context("error reading pxar archive")?; > +struct ExtractorIterState { > + match_stack: Vec<bool>, > + err_path_stack: Vec<OsString>, > + current_match: bool, > + end_reached: bool, > +} > > - if !root.is_dir() { > - bail!("pxar archive does not start with a directory entry!"); > +/// An [`Iterator`] that encapsulates the process of extraction in [extract_archive]. > +/// Therefore, traversing over an [`ExtractorIter`] until exhaustion extracts an > +/// entire PXAR archive. > +struct ExtractorIter<'a, T, F> > +where > + T: pxar::decoder::SeqRead, > + F: FnMut(&Path), > +{ > + decoder: pxar::decoder::Decoder<T>, > + callback: F, > + extractor: Extractor, > + match_list: &'a [MatchEntry], > + state: ExtractorIterState, > +} > + > +impl ExtractorIterState { > + fn new(options: &PxarExtractOptions) -> Self { > + Self { > + match_stack: Vec::new(), > + err_path_stack: Vec::new(), > + current_match: options.extract_match_default, > + end_reached: false, > + } > } > +} > > - create_path( > - destination, > - None, > - Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), > - ) > - .context(format!("error creating directory {:?}", destination))?; > +impl<'a, T, F> ExtractorIter<'a, T, F> > +where > + T: pxar::decoder::SeqRead, > + F: FnMut(&Path), > +{ > + /// Creates and initializes the state of a new [`ExtractorIter`]. > + /// > + /// This function requires that the given [`Decoder`][D] has not made a single > + /// traversal (a call to [`next()`][next]) yet. > + /// > + /// [D]: pxar::decoder::Decoder > + /// [next]: std::iter::Iterator::next() > + fn new( > + mut decoder: pxar::decoder::Decoder<T>, > + destination: &Path, > + feature_flags: Flags, > + callback: F, > + options: PxarExtractOptions<'a>, > + ) -> Result<Self, Error> { > + // we use this to keep track of our directory-traversal > + decoder.enable_goodbye_entries(true); > + > + let root = decoder > + .next() > + .context("found empty pxar archive")? > + .context("error reading pxar archive")?; > + > + if !root.is_dir() { > + bail!("pxar archive does not start with a directory entry!"); > + } > > - let dir = Dir::open( > - destination, > - OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, > - Mode::empty(), > - ) > - .context(format!("unable to open target directory {:?}", destination))?; > - > - let mut extractor = Extractor::new( > - dir, > - root.metadata().clone(), > - options.allow_existing_dirs, > - options.overwrite, > - feature_flags, > - ); > - > - if let Some(on_error) = options.on_error { > - extractor.on_error(on_error); > + let mut state = ExtractorIterState::new(&options); > + state.err_path_stack.push(OsString::from("/")); > + > + create_path( > + destination, > + None, > + Some(CreateOptions::new().perm(Mode::from_bits_truncate(0o700))), > + ) > + .context(format!("error creating directory {destination:?}"))?; > + > + let dir = Dir::open( > + destination, > + OFlag::O_DIRECTORY | OFlag::O_CLOEXEC, > + Mode::empty(), > + ) > + .context(format!("unable to open target directory {destination:?}"))?; > + > + let mut extractor = Extractor::new( > + dir, > + root.metadata().clone(), > + options.allow_existing_dirs, > + options.overwrite, > + feature_flags, > + ); > + > + if let Some(on_error) = options.on_error { > + extractor.on_error(on_error); > + } > + > + Ok(Self { > + decoder, > + callback, > + extractor, > + match_list: options.match_list, > + state, > + }) > } > > - let mut match_stack = Vec::new(); > - 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.context("error reading pxar archive")?; > + #[inline(always)] > + fn callback(&mut self, path: &Path) { > + (self.callback)(path) > + } > > - let file_name_os = entry.file_name(); > + /// Yields the [`Decoder`][D]'s [`Result`]s and validates each [`Entry`][E] > + /// it encounters. It also controls the [`ExtractorIter`]'s stop condition. > + /// > + /// In detail, the [`ExtractorIter`] will stop if and only if one of the > + /// following conditions is true: > + /// * The [`Decoder`][D] is exhausted > + /// * The [`Decoder`][D] failed to read from the archive and consequently > + /// yielded an [`io::Error`] > + /// * Validation of an [`Entry`][E] failed > + /// > + /// The iterator will yield the encountered error before stopping. > + /// > + /// [D]: pxar::decoder::Decoder > + /// [E]: pxar::Entry > + #[inline] > + fn next_entry(&mut self) -> Option<Result<Entry, Error>> { > + if self.state.end_reached { > + return None; > + } > > - // safety check: a file entry in an archive must never contain slashes: > - if file_name_os.as_bytes().contains(&b'/') { > - bail!("archive file entry contains slashes, which is invalid and a security concern"); > + let entry = self.decoder.next(); > + > + if entry.is_none() { ^ This could be a diverging `match` to avoid the `unwrap` below. let entry = match entry { None => { .. the code from here .. } Some(entry) => entry, }; (side note: too much code for a `let else` to be formatted properly) > + self.state.end_reached = true; > + if !self.extractor.dir_stack.is_empty() { > + return Some(Err(anyhow!("unexpected eof while decoding pxar archive"))); style: we typically use `format_err!`, I don't think we should start mixing in `anyhow!()` unless there's a good reason for it. > + } else { > + return None; > + } > } > > - let file_name = CString::new(file_name_os.as_bytes()) > - .context("encountered file name with null-bytes")?; > + #[inline(always)] > + fn validate_entry(entry: Result<Entry, io::Error>) -> Result<Entry, Error> { > + let entry = entry.context("error reading pxar archive")?; > + > + let file_name_bytes = entry.file_name().as_bytes(); > + > + if file_name_bytes.contains(&b'/') { > + bail!( > + "archive file entry contains slashes, which is invalid and a security concern" > + ); > + } > + > + CString::new(file_name_bytes).context("encountered file name with null-bytes")?; ^ I'd rather drop this here, because this allocates for no reason other than to check for errors, both of which happens again later on. As a side note, do we even need a `CString`? Or can the whole code using `file_name` below work on a `CStr` as well? If you insist on checking here - at least use `CStr::from_bytes_with_nul` here - add a comment about the code below - use an `unsafe{}` block with a comment below with `CStr::from_bytes_with_nul_unchecked` But only checking later makes a lot more sense to me :-) > + > + Ok(entry) > + } > + > + let entry = validate_entry(entry.unwrap()); > + > + if entry.is_err() { > + self.state.end_reached = true; > + } > > + Some(entry) > + } > +} > + > +impl<'a, T, F> Iterator for ExtractorIter<'a, T, F> > +where > + T: pxar::decoder::SeqRead, > + F: FnMut(&Path), > +{ > + type Item = Result<(), Error>; > + > + /// Performs the actual extraction of decoded [`Entries`][E]. > + /// > + /// Should an error occur during any point of extraction (**not** while > + /// fetching the next [`Entry`][E]), the error may be handled by the > + /// [`ErrorHandler`] provided by the [`PxarExtractOptions`] used to > + /// initialize the iterator. > + /// > + /// [E]: pxar::Entry > + fn next(&mut self) -> Option<Self::Item> { > + let entry = self.next_entry()?; > + > + if entry.is_err() { > + assert!( > + self.state.end_reached, > + "encountered an error while reading the archive, but the iterator was not stopped!" > + ); > + > + return Some(entry.map(drop)); > + } > + > + let entry = entry.unwrap(); ^ Same as in `next_entry` - we can easily get rid of the `unwrap` with a `match`. > + let file_name_os = entry.file_name(); > let metadata = entry.metadata(); > > - extractor.set_path(entry.path().as_os_str().to_owned()); > + // can unwrap here because next_entry already checked for null bytes > + let file_name = CString::new(file_name_os.as_bytes()).unwrap(); > + > + self.extractor.set_path(entry.path().as_os_str().to_owned()); > > - let match_result = options.match_list.matches( > + let match_result = self.match_list.matches( > entry.path().as_os_str().as_bytes(), > Some(metadata.file_type() as u32), > ); > @@ -116,99 +265,114 @@ where > let did_match = match match_result { > Some(MatchType::Include) => true, > Some(MatchType::Exclude) => false, > - None => current_match, > + None => self.state.current_match, > }; > - match (did_match, entry.kind()) { > - (_, EntryKind::Directory) => { > - callback(entry.path()); > - > - let create = current_match && match_result != Some(MatchType::Exclude); > - extractor > - .enter_directory(file_name_os.to_owned(), metadata.clone(), create) > - .context(format!("error at entry {:?}", file_name_os))?; > > - // We're starting a new directory, push our old matching state and replace it with > - // our new one: > - match_stack.push(current_match); > - current_match = did_match; > - > - // When we hit the goodbye table we'll try to apply metadata to the directory, but > - // the Goodbye entry will not contain the path, so push it to our path stack for > - // error messages: > - err_path_stack.push(extractor.clone_path()); > + let extract_res = match (did_match, entry.kind()) { > + (_, EntryKind::Directory) => { > + self.callback(entry.path()); > + > + let create = self.state.current_match && match_result != Some(MatchType::Exclude); > + let res = self.extractor.enter_directory( > + file_name_os.to_owned(), > + metadata.clone(), > + create, > + ); > + > + if res.is_ok() { > + // We're starting a new directory, push our old matching state and replace it with > + // our new one: > + self.state.match_stack.push(self.state.current_match); > + self.state.current_match = did_match; > + > + // When we hit the goodbye table we'll try to apply metadata to the directory, but > + // the Goodbye entry will not contain the path, so push it to our path stack for > + // error messages: > + self.state.err_path_stack.push(self.extractor.clone_path()); > + } > > - Ok(()) > + res > } > (_, EntryKind::GoodbyeTable) => { > // go up a directory > > - extractor.set_path(err_path_stack.pop().context(format!( > - "error at entry {:?} - unexpected end of directory", > - file_name_os > - ))?); > - > - extractor > - .leave_directory() > - .context(format!("error at entry {:?}", file_name_os))?; > - > - // We left a directory, also get back our previous matching state. This is in sync > - // with `dir_stack` so this should never be empty except for the final goodbye > - // table, in which case we get back to the default of `true`. > - current_match = match_stack.pop().unwrap_or(true); > + let res = self > + .state > + .err_path_stack > + .pop() > + .context("unexpected end of directory") > + .map(|path| self.extractor.set_path(path)) > + .and(self.extractor.leave_directory()); > + > + if res.is_ok() { > + // We left a directory, also get back our previous matching state. This is in sync > + // with `dir_stack` so this should never be empty except for the final goodbye > + // table, in which case we get back to the default of `true`. > + self.state.current_match = self.state.match_stack.pop().unwrap_or(true); > + } > > - Ok(()) > + res > } > (true, EntryKind::Symlink(link)) => { > - callback(entry.path()); > - extractor.extract_symlink(&file_name, metadata, link.as_ref()) > + self.callback(entry.path()); > + self.extractor > + .extract_symlink(&file_name, metadata, link.as_ref()) > } > (true, EntryKind::Hardlink(link)) => { > - callback(entry.path()); > - extractor.extract_hardlink(&file_name, link.as_os_str()) > + self.callback(entry.path()); > + self.extractor > + .extract_hardlink(&file_name, link.as_os_str()) > } > (true, EntryKind::Device(dev)) => { > - if extractor.contains_flags(Flags::WITH_DEVICE_NODES) { > - callback(entry.path()); > - extractor.extract_device(&file_name, metadata, dev) > + if self.extractor.contains_flags(Flags::WITH_DEVICE_NODES) { > + self.callback(entry.path()); > + self.extractor.extract_device(&file_name, metadata, dev) > } else { > Ok(()) > } > } > (true, EntryKind::Fifo) => { > - if extractor.contains_flags(Flags::WITH_FIFOS) { > - callback(entry.path()); > - extractor.extract_special(&file_name, metadata, 0) > + if self.extractor.contains_flags(Flags::WITH_FIFOS) { > + self.callback(entry.path()); > + self.extractor.extract_special(&file_name, metadata, 0) > } else { > Ok(()) > } > } > (true, EntryKind::Socket) => { > - if extractor.contains_flags(Flags::WITH_SOCKETS) { > - callback(entry.path()); > - extractor.extract_special(&file_name, metadata, 0) > + if self.extractor.contains_flags(Flags::WITH_SOCKETS) { > + self.callback(entry.path()); > + self.extractor.extract_special(&file_name, metadata, 0) > } else { > Ok(()) > } > } > - (true, EntryKind::File { size, .. }) => extractor.extract_file( > - &file_name, > - metadata, > - *size, > - &mut decoder > - .contents() > - .context("found regular file entry without contents in archive")?, > - extractor.overwrite, > - ), > + (true, EntryKind::File { size, .. }) => { > + let contents = self.decoder.contents(); > + > + if let Some(mut contents) = contents { > + self.extractor.extract_file( > + &file_name, > + metadata, > + *size, > + &mut contents, > + self.extractor.overwrite, > + ) > + } else { > + Err(anyhow!( > + "found regular file entry without contents in archive" > + )) > + } > + } > (false, _) => Ok(()), // skip this > - } > - .context(format!("error at entry {:?}", file_name_os))?; > - } > + }; > > - if !extractor.dir_stack.is_empty() { > - bail!("unexpected eof while decoding pxar archive"); > + Some( > + extract_res > + .context(format!("error at entry {file_name_os:?}")) > + .or_else(&mut *self.extractor.on_error), > + ) > } > - > - Ok(()) > } > > /// Common state for file extraction. > -- > 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/5] pbs-client: pxar: add `PxarExtractContext` 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` Max Carrara @ 2023-06-07 16:34 ` Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs Max Carrara 4 siblings, 0 replies; 10+ messages in thread From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw) To: pbs-devel This enum's purpose is to provide context to errors that occur during the extraction of a pxar archive, making it possible to handle extraction errors in a more granular manner. For now, it's only implemented in `ExtractorIter::next()`, but may be used in other places if necessary or desired. Signed-off-by: Max Carrara <m.carrara@proxmox.com> --- Notes: I'm intentionally only using `PxarExtractContext` within the `ExtractorIter` here, as I'm not certain whether any other places could benefit from the same type of context variable as well (e.g. the async fns further down in extract.rs). pbs-client/src/pxar/extract.rs | 98 ++++++++++++++++++++++++++++++---- pbs-client/src/pxar/mod.rs | 2 +- 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs index fa08bfd7..6b026a02 100644 --- a/pbs-client/src/pxar/extract.rs +++ b/pbs-client/src/pxar/extract.rs @@ -235,6 +235,8 @@ where /// [`ErrorHandler`] provided by the [`PxarExtractOptions`] used to /// initialize the iterator. /// + /// Extraction errors will have a corresponding [`PxarExtractContext`] attached. + /// /// [E]: pxar::Entry fn next(&mut self) -> Option<Self::Item> { let entry = self.next_entry()?; @@ -273,11 +275,10 @@ where self.callback(entry.path()); let create = self.state.current_match && match_result != Some(MatchType::Exclude); - let res = self.extractor.enter_directory( - file_name_os.to_owned(), - metadata.clone(), - create, - ); + let res = self + .extractor + .enter_directory(file_name_os.to_owned(), metadata.clone(), create) + .context(PxarExtractContext::EnterDirectory); if res.is_ok() { // We're starting a new directory, push our old matching state and replace it with @@ -302,7 +303,8 @@ where .pop() .context("unexpected end of directory") .map(|path| self.extractor.set_path(path)) - .and(self.extractor.leave_directory()); + .and(self.extractor.leave_directory()) + .context(PxarExtractContext::LeaveDirectory); if res.is_ok() { // We left a directory, also get back our previous matching state. This is in sync @@ -317,16 +319,20 @@ where self.callback(entry.path()); self.extractor .extract_symlink(&file_name, metadata, link.as_ref()) + .context(PxarExtractContext::ExtractSymlink) } (true, EntryKind::Hardlink(link)) => { self.callback(entry.path()); self.extractor .extract_hardlink(&file_name, link.as_os_str()) + .context(PxarExtractContext::ExtractHardlink) } (true, EntryKind::Device(dev)) => { if self.extractor.contains_flags(Flags::WITH_DEVICE_NODES) { self.callback(entry.path()); - self.extractor.extract_device(&file_name, metadata, dev) + self.extractor + .extract_device(&file_name, metadata, dev) + .context(PxarExtractContext::ExtractDevice) } else { Ok(()) } @@ -334,7 +340,9 @@ where (true, EntryKind::Fifo) => { if self.extractor.contains_flags(Flags::WITH_FIFOS) { self.callback(entry.path()); - self.extractor.extract_special(&file_name, metadata, 0) + self.extractor + .extract_special(&file_name, metadata, 0) + .context(PxarExtractContext::ExtractFifo) } else { Ok(()) } @@ -342,7 +350,9 @@ where (true, EntryKind::Socket) => { if self.extractor.contains_flags(Flags::WITH_SOCKETS) { self.callback(entry.path()); - self.extractor.extract_special(&file_name, metadata, 0) + self.extractor + .extract_special(&file_name, metadata, 0) + .context(PxarExtractContext::ExtractSocket) } else { Ok(()) } @@ -363,6 +373,7 @@ where "found regular file entry without contents in archive" )) } + .context(PxarExtractContext::ExtractFile) } (false, _) => Ok(()), // skip this }; @@ -375,6 +386,75 @@ where } } +/// Provides additional [context][C] for [`anyhow::Error`]s that are returned +/// while traversing an [`ExtractorIter`]. The [`PxarExtractContext`] can then +/// be accessed [via `anyhow`'s facilities][A] and may aid during error handling. +/// +/// +/// # Example +/// +/// ``` +/// # use anyhow::{anyhow, Error}; +/// # use std::io; +/// # use pbs_client::pxar::PxarExtractContext; +/// +/// let err = anyhow!("oh noes!").context(PxarExtractContext::ExtractFile); +/// +/// if let Some(ctx) = err.downcast_ref::<PxarExtractContext>() { +/// match ctx { +/// PxarExtractContext::ExtractFile => { +/// // Conditionally handle the underlying error by type +/// if let Some(io_err) = err.downcast_ref::<io::Error>() { +/// // ... +/// }; +/// }, +/// PxarExtractContext::ExtractSocket => { +/// // ... +/// }, +/// // ... +/// # _ => (), +/// } +/// } +/// ``` +/// +/// [A]: anyhow::Error +/// [C]: anyhow::Context +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum PxarExtractContext { + EnterDirectory, + LeaveDirectory, + ExtractSymlink, + ExtractHardlink, + ExtractDevice, + ExtractFifo, + ExtractSocket, + ExtractFile, +} + +impl PxarExtractContext { + #[inline] + pub fn as_str(&self) -> &'static str { + use PxarExtractContext::*; + + match *self { + EnterDirectory => "failed to enter directory", + LeaveDirectory => "failed to leave directory", + ExtractSymlink => "failed to extract symlink", + ExtractHardlink => "failed to extract hardlink", + ExtractDevice => "failed to extract device", + ExtractFifo => "failed to extract named pipe", + ExtractSocket => "failed to extract unix socket", + ExtractFile => "failed to extract file", + } + } +} + +impl std::fmt::Display for PxarExtractContext { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + /// Common state for file extraction. pub struct Extractor { feature_flags: Flags, diff --git a/pbs-client/src/pxar/mod.rs b/pbs-client/src/pxar/mod.rs index a158101d..b042717d 100644 --- a/pbs-client/src/pxar/mod.rs +++ b/pbs-client/src/pxar/mod.rs @@ -59,7 +59,7 @@ pub use flags::Flags; pub use create::{create_archive, PxarCreateOptions}; pub use extract::{ create_tar, create_zip, extract_archive, extract_sub_dir, extract_sub_dir_seq, ErrorHandler, - PxarExtractOptions, + PxarExtractContext, PxarExtractOptions, }; /// The format requires to build sorted directory lookup tables in -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara ` (2 preceding siblings ...) 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 3/5] pbs-client: pxar: add `PxarExtractContext` Max Carrara @ 2023-06-07 16:34 ` Max Carrara 2023-07-11 14:17 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs Max Carrara 4 siblings, 1 reply; 10+ messages in thread From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw) To: pbs-devel If this flag is provided, any errors that occur during the extraction of a device node are silently ignored. In addition to this flag and its error handler, the bare scaffold for supporting multiple error handlers is also added. Signed-off-by: Max Carrara <m.carrara@proxmox.com> --- Notes: One thing I'm not *really* sure about here is the name of the CLI flag - to me it feels a little bit too verbose, but it does exactly what it says. proxmox-backup-client/src/main.rs | 61 ++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs index 55198108..1fb8a378 100644 --- a/proxmox-backup-client/src/main.rs +++ b/proxmox-backup-client/src/main.rs @@ -29,6 +29,7 @@ use pbs_api_types::{ BACKUP_TYPE_SCHEMA, TRAFFIC_CONTROL_BURST_SCHEMA, TRAFFIC_CONTROL_RATE_SCHEMA, }; use pbs_client::catalog_shell::Shell; +use pbs_client::pxar::ErrorHandler as PxarErrorHandler; use pbs_client::tools::{ complete_archive_name, complete_auth_id, complete_backup_group, complete_backup_snapshot, complete_backup_source, complete_chunk_size, complete_group_or_snapshot, @@ -1232,6 +1233,12 @@ We do not extract '.pxar' archives when writing to standard output. optional: true, default: false, }, + "ignore-extract-device-errors": { + type: Boolean, + description: "ignore errors that occur during device node extraction", + optional: true, + default: false, + } } } )] @@ -1244,6 +1251,7 @@ async fn restore( ignore_ownership: bool, ignore_permissions: bool, overwrite: bool, + ignore_extract_device_errors: bool, ) -> Result<Value, Error> { let repo = extract_repository_from_value(¶m)?; @@ -1364,12 +1372,63 @@ async fn restore( let mut reader = BufferedDynamicReader::new(index, chunk_reader); + fn make_handler_from_many<I>(handlers: I) -> PxarErrorHandler + where + I: IntoIterator<Item = PxarErrorHandler>, + { + let mut captured_handlers: Vec<PxarErrorHandler> = handlers.into_iter().collect(); + + let handler: PxarErrorHandler = Box::new(move |error: Error| -> Result<(), Error> { + let mut res = Err(error); + + for handler in captured_handlers.iter_mut() { + if let Err(error) = res { + res = handler(error); + } else { + return res; + } + } + + res + }); + + handler + } + + let mut handlers: Vec<PxarErrorHandler> = Vec::new(); + + if ignore_extract_device_errors { + let on_device_error = Box::new(move |err: Error| { + use pbs_client::pxar::PxarExtractContext; + + let ctx = err.downcast_ref::<PxarExtractContext>(); + + if ctx.is_none() { + return Err(err); + } + + if matches!(ctx.unwrap(), PxarExtractContext::ExtractDevice) { + return Ok(()); + } + + Err(err) + }); + + handlers.push(on_device_error); + } + + let on_error = if handlers.is_empty() { + None + } else { + Some(make_handler_from_many(handlers)) + }; + let options = pbs_client::pxar::PxarExtractOptions { match_list: &[], extract_match_default: true, allow_existing_dirs, overwrite, - on_error: None, + on_error, }; let mut feature_flags = pbs_client::pxar::Flags::DEFAULT; -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag Max Carrara @ 2023-07-11 14:17 ` Wolfgang Bumiller 0 siblings, 0 replies; 10+ messages in thread From: Wolfgang Bumiller @ 2023-07-11 14:17 UTC (permalink / raw) To: Max Carrara; +Cc: pbs-devel On Wed, Jun 07, 2023 at 06:34:27PM +0200, Max Carrara wrote: > If this flag is provided, any errors that occur during the extraction > of a device node are silently ignored. > > In addition to this flag and its error handler, the bare scaffold > for supporting multiple error handlers is also added. > > Signed-off-by: Max Carrara <m.carrara@proxmox.com> > --- > > Notes: > One thing I'm not *really* sure about here is the name of the CLI > flag - to me it feels a little bit too verbose, but it does exactly > what it says. > > proxmox-backup-client/src/main.rs | 61 ++++++++++++++++++++++++++++++- > 1 file changed, 60 insertions(+), 1 deletion(-) > > diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs > index 55198108..1fb8a378 100644 > --- a/proxmox-backup-client/src/main.rs > +++ b/proxmox-backup-client/src/main.rs > @@ -29,6 +29,7 @@ use pbs_api_types::{ > BACKUP_TYPE_SCHEMA, TRAFFIC_CONTROL_BURST_SCHEMA, TRAFFIC_CONTROL_RATE_SCHEMA, > }; > use pbs_client::catalog_shell::Shell; > +use pbs_client::pxar::ErrorHandler as PxarErrorHandler; > use pbs_client::tools::{ > complete_archive_name, complete_auth_id, complete_backup_group, complete_backup_snapshot, > complete_backup_source, complete_chunk_size, complete_group_or_snapshot, > @@ -1232,6 +1233,12 @@ We do not extract '.pxar' archives when writing to standard output. > optional: true, > default: false, > }, > + "ignore-extract-device-errors": { > + type: Boolean, > + description: "ignore errors that occur during device node extraction", > + optional: true, > + default: false, > + } > } > } > )] > @@ -1244,6 +1251,7 @@ async fn restore( > ignore_ownership: bool, > ignore_permissions: bool, > overwrite: bool, > + ignore_extract_device_errors: bool, > ) -> Result<Value, Error> { > let repo = extract_repository_from_value(¶m)?; > > @@ -1364,12 +1372,63 @@ async fn restore( > > let mut reader = BufferedDynamicReader::new(index, chunk_reader); > > + fn make_handler_from_many<I>(handlers: I) -> PxarErrorHandler > + where > + I: IntoIterator<Item = PxarErrorHandler>, > + { > + let mut captured_handlers: Vec<PxarErrorHandler> = handlers.into_iter().collect(); > + > + let handler: PxarErrorHandler = Box::new(move |error: Error| -> Result<(), Error> { > + let mut res = Err(error); > + > + for handler in captured_handlers.iter_mut() { > + if let Err(error) = res { > + res = handler(error); > + } else { > + return res; > + } > + } > + > + res > + }); > + > + handler > + } > + > + let mut handlers: Vec<PxarErrorHandler> = Vec::new(); > + > + if ignore_extract_device_errors { > + let on_device_error = Box::new(move |err: Error| { > + use pbs_client::pxar::PxarExtractContext; > + > + let ctx = err.downcast_ref::<PxarExtractContext>(); > + > + if ctx.is_none() { Another `is_none()` check + unwrap. How about match err.downcast_ref::<PxarExtractContext>() { Some(PxarExtractContext::ExtractDevice) => Ok(()), _ => Err(err), ) ? :) > + return Err(err); > + } > + > + if matches!(ctx.unwrap(), PxarExtractContext::ExtractDevice) { > + return Ok(()); > + } > + > + Err(err) > + }); > + > + handlers.push(on_device_error); > + } > + > + let on_error = if handlers.is_empty() { > + None > + } else { > + Some(make_handler_from_many(handlers)) > + }; > + > let options = pbs_client::pxar::PxarExtractOptions { > match_list: &[], > extract_match_default: true, > allow_existing_dirs, > overwrite, > - on_error: None, > + on_error, > }; > > let mut feature_flags = pbs_client::pxar::Flags::DEFAULT; > -- > 2.30.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara ` (3 preceding siblings ...) 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag Max Carrara @ 2023-06-07 16:34 ` Max Carrara 2023-06-08 13:47 ` Thomas Lamprecht 4 siblings, 1 reply; 10+ messages in thread From: Max Carrara @ 2023-06-07 16:34 UTC (permalink / raw) To: pbs-devel Signed-off-by: Max Carrara <m.carrara@proxmox.com> --- NOTE: This patch requires a version bump+upload of proxmox-backup-client. src/PVE/LXC/Create.pm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm index b2e3d00..8275674 100644 --- a/src/PVE/LXC/Create.pm +++ b/src/PVE/LXC/Create.pm @@ -116,6 +116,10 @@ sub restore_proxmox_backup_archive { my $cmd = "restore"; my $param = [$name, "root.pxar", $rootdir, '--allow-existing-dirs']; + if ($no_unpack_error) { + push(@$param, '--ignore-extract-device-errors'); + } + PVE::Storage::PBSPlugin::run_raw_client_cmd( $scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd); -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs 2023-06-07 16:34 ` [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs Max Carrara @ 2023-06-08 13:47 ` Thomas Lamprecht 0 siblings, 0 replies; 10+ messages in thread From: Thomas Lamprecht @ 2023-06-08 13:47 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Max Carrara On 07/06/2023 18:34, Max Carrara wrote: > Signed-off-by: Max Carrara <m.carrara@proxmox.com> > --- > NOTE: This patch requires a version bump+upload of > proxmox-backup-client. and bumping the versioned dependency in debian/control to actually guarantee the right version is co-installed. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-07-11 14:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-07 16:34 [pbs-devel] [PATCH proxmox-backup/pve-container 0/5] improve error handling of pxar extractor Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 1/5] pbs-client: pxar: preserve error context Max Carrara 2023-07-11 12:54 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 2/5] pbs-client: pxar: refactor body of `extract_archive` to `ExtractorIter` Max Carrara 2023-07-11 13:24 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 3/5] pbs-client: pxar: add `PxarExtractContext` Max Carrara 2023-06-07 16:34 ` [pbs-devel] [PATCH proxmox-backup 4/5] proxmox-backup-client: restore: add 'ignore-extract-device-errors' flag Max Carrara 2023-07-11 14:17 ` Wolfgang Bumiller 2023-06-07 16:34 ` [pbs-devel] [PATCH pve-container 5/5] fix #3460: restore: honor '--ignore-unpack-errors' flag for pbs Max Carrara 2023-06-08 13:47 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox