Re: [pbs-devel] [PATCH v3 proxmox-backup 4/6] datastore: add helper method to open index reader from path

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

minor error handling nits:

On Thu, Mar 20, 2025 at 01:30:08PM +0100, Christian Ebner wrote:
> Refactor the archive type and index file reader opening with its
> error handling into a helper method for better reusability.
> 
> This allows to use the same logic for both, expected image paths
> and unexpected image paths when iterating trough the datastore
> in a hierarchical manner.
> 
> Improve error handling by switching to anyhow's error context.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 1:
> - add check for passed path being absolute
> 
>  pbs-datastore/src/datastore.rs | 76 ++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 22 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index a6a91ca79..7b5ea4272 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -5,7 +5,7 @@ use std::os::unix::io::AsRawFd;
>  use std::path::{Path, PathBuf};
>  use std::sync::{Arc, LazyLock, Mutex};
>  
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, format_err, Context, Error};
>  use nix::unistd::{unlinkat, UnlinkatFlags};
>  use tracing::{info, warn};
>  
> @@ -1029,10 +1029,57 @@ impl DataStore {
>          Ok(list)
>      }
>  
> +    // Similar to open index, but ignore index files with blob or unknown archive type.
> +    // Further, do not fail if file vanished.
> +    fn open_index_reader(&self, absolute_path: &Path) -> Result<Option<Box<dyn IndexFile>>, Error> {
> +        let archive_type = match ArchiveType::from_path(absolute_path) {
> +            Ok(archive_type) => archive_type,
> +            // ignore archives with unknown archive type
> +            Err(_) => return Ok(None),
> +        };
> +
> +        if absolute_path.is_relative() {
> +            bail!(
> +                "expected absolute path, got '{}'",
> +                absolute_path.to_string_lossy()

Since this get touched anyway - I think it makes sense (and it's much
less code) to just format paths with `{:?}` (and inline them into the
format string ->
    bail!("expected absolute path, got {absolute_path:?}");
is a single line after all ;-) )

> +            );
> +        }
> +
> +        let file = match std::fs::File::open(absolute_path) {
> +            Ok(file) => file,
> +            // ignore vanished files
> +            Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None),
> +            Err(err) => {
> +                return Err(Error::from(err).context(format!(
> +                    "can't open file {}",
> +                    absolute_path.to_string_lossy()
> +                )))
> +            }
> +        };
> +
> +        match archive_type {
> +            ArchiveType::FixedIndex => {
> +                let reader = FixedIndexReader::new(file).context(format!(

`format!()` allocates, so this should use `.with_context()` instead of
`.context()` to only do that when an actual error happens.


> +                    "can't open fixed index {}",
> +                    absolute_path.to_string_lossy()
> +                ))?;
> +                Ok(Some(Box::new(reader)))
> +            }
> +            ArchiveType::DynamicIndex => {
> +                let reader = DynamicIndexReader::new(file).context(format!(

^ same here

> +                    "can't open dynamic index {}",
> +                    absolute_path.to_string_lossy()
> +                ))?;
> +                Ok(Some(Box::new(reader)))
> +            }
> +            ArchiveType::Blob => Ok(None),
> +        }
> +    }
> +
>      // mark chunks  used by ``index`` as used
> -    fn index_mark_used_chunks<I: IndexFile>(
> +    fn index_mark_used_chunks(
>          &self,
> -        index: I,
> +        index: Box<dyn IndexFile>,
>          file_name: &Path, // only used for error reporting
>          status: &mut GarbageCollectionStatus,
>          worker: &dyn WorkerTaskContext,
> @@ -1090,24 +1137,8 @@ impl DataStore {
>                  }
>              }
>  
> -            match std::fs::File::open(&img) {
> -                Ok(file) => {
> -                    if let Ok(archive_type) = ArchiveType::from_path(&img) {
> -                        if archive_type == ArchiveType::FixedIndex {
> -                            let index = FixedIndexReader::new(file).map_err(|e| {
> -                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
> -                            })?;
> -                            self.index_mark_used_chunks(index, &img, status, worker)?;
> -                        } else if archive_type == ArchiveType::DynamicIndex {
> -                            let index = DynamicIndexReader::new(file).map_err(|e| {
> -                                format_err!("can't read index '{}' - {}", img.to_string_lossy(), e)
> -                            })?;
> -                            self.index_mark_used_chunks(index, &img, status, worker)?;
> -                        }
> -                    }
> -                }
> -                Err(err) if err.kind() == io::ErrorKind::NotFound => (), // ignore vanished files
> -                Err(err) => bail!("can't open index {} - {}", img.to_string_lossy(), err),
> +            if let Some(index) = self.open_index_reader(&img)? {
> +                self.index_mark_used_chunks(index, &img, status, worker)?;
>              }
>  
>              let percentage = (i + 1) * 100 / image_count;
> @@ -1173,7 +1204,8 @@ impl DataStore {
>  
>              info!("Start GC phase1 (mark used chunks)");
>  
> -            self.mark_used_chunks(&mut gc_status, worker)?;
> +            self.mark_used_chunks(&mut gc_status, worker)
> +                .context("marking used chunks failed")?;
>  
>              info!("Start GC phase2 (sweep unused chunks)");
>              self.inner.chunk_store.sweep_unused_chunks(
> -- 
> 2.39.5


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel