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

[Christian Ebner <c.ebner@proxmox.com>]

On 3/17/25 15:59, Fabian Grünbichler wrote:
> 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?

Acked, will adapt this accordingly!

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

Ok, will dot this as followup patches on top of this as well.

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



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