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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 10, 2025 12:16 pm, 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:
> - not present in previous version
> 
>  pbs-datastore/src/datastore.rs | 66 ++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index a6a91ca79..72bc9f77f 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,47 @@ 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.

nit: but compared to open_index it takes an absolute path, not a relative one
to the base of the datastore? this should probably be made explicit and
checked?

(it might also at some point make sense to pull out GC+related helpers
into a separate file to separate such things properly..)

> +    fn open_index_reader(&self, path: &Path) -> Result<Option<Box<dyn IndexFile>>, Error> {
> +        let archive_type = match ArchiveType::from_path(path) {
> +            Ok(archive_type) => archive_type,
> +            // ignore archives with unknown archive type
> +            Err(_) => return Ok(None),
> +        };
> +
> +        let file = match std::fs::File::open(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 {}", path.to_string_lossy()))
> +                )
> +            }
> +        };
> +
> +        match archive_type {
> +            ArchiveType::FixedIndex => {
> +                let reader = FixedIndexReader::new(file)
> +                    .context(format!("can't open fixed index {}", path.to_string_lossy()))?;
> +                Ok(Some(Box::new(reader)))
> +            }
> +            ArchiveType::DynamicIndex => {
> +                let reader = DynamicIndexReader::new(file).context(format!(
> +                    "can't open dynamic index {}",
> +                    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 +1127,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 +1194,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)
> +                .map_err(|err| format_err!("marking used chunks failed - {err:#}"))?;
>  
>              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
> 
> 
> 


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