Re: [PATCH proxmox-backup 16/28] datastore: conditionally treat missing manifest as error or bening

[Robert Obkircher <r.obkircher@proxmox.com>]

> A backup snapshot without a manifest is generally a transient state
> during ongoing backups. In other cases it must however be treated
> as error.
> 
> Therefore, refactor the manifest and blob loading logic to
> conditionally allow for missing manifest blob files. By this the
> caller is in control whether to treat the missing file as error or
> regular operation.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index be4ec8b3e..f476d4469 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -648,11 +648,26 @@ impl BackupDir {
>          let mut path = self.full_path();
>          path.push(filename);
>  
> -        proxmox_lang::try_block!({
> -            let mut file = std::fs::File::open(&path)?;
> -            DataBlob::load_from_reader(&mut file)
> -        })
> -        .map_err(|err| format_err!("unable to load blob '{:?}' - {}", path, err))
> +        self.load_blob_conditionally(&path, true)
> +            .map_err(|err| format_err!("unable to load blob '{path:?}' - {err}"))?
> +            .ok_or(format_err!("unable to load missing blob '{path:?}'"))
The second one is unreachable, because not_found_is_error was true.
I think the boolean parameter could just be removed entirely.
> +    }
> +
> +    /// Load the `DataBlob` from given full path, to be constructed and verified by the caller.
> +    ///
> +    /// If the blob file does not exist and `not_found_is_error` is `true`, propagates the error
> +    /// while otherwise `Ok(None)` is returned.
> +    fn load_blob_conditionally(
> +        &self,
> +        full_path: &Path,
> +        not_found_is_error: bool,
> +    ) -> Result<Option<DataBlob>, Error> {
> +        let opt_data_blob = match std::fs::File::open(full_path) {
> +            Ok(mut file) => Some(DataBlob::load_from_reader(&mut file)?),
> +            Err(err) if err.kind() == std::io::ErrorKind::NotFound && !not_found_is_error => None,
> +            Err(err) => return Err(err.into()),
> +        };
> +        Ok(opt_data_blob)
>      }
>  
>      /// Generate the full archive file path with given archive name including server side type
> @@ -989,11 +1004,34 @@ impl BackupDir {
>      }
>  
>      /// Load the manifest without a lock. Must not be written back.
> +    ///
> +    /// A missing manifest file for the snapshot is treated as error.
>      pub fn load_manifest(&self) -> Result<(BackupManifest, u64), Error> {
> -        let blob = self.load_blob(MANIFEST_BLOB_NAME.as_ref())?;
> +        self.load_manifest_optionally(true)?.ok_or_else(|| {
> +            format_err!(
> +                "unable to load missing manifest '{:?}'",
> +                self.full_path().join(MANIFEST_BLOB_NAME.as_ref()),
> +            )
Same here.

-- 
Robert Obkircher <r.obkircher@proxmox.com>