Re: [pbs-devel] [PATCH proxmox-backup v3 01/13] pbs-datastore: add protection info to BackupInfo

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

minor nitpicking below

On Wed, Oct 27, 2021 at 01:22:26PM +0200, Dominik Csapak wrote:
> and add necessary helper functions (protected_file/is_protected)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/backup_info.rs | 20 ++++++++++++++++++--
>  tests/prune.rs                   |  2 +-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 9d1ab991..b94b9779 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -92,7 +92,9 @@ impl BackupGroup {
>                      BackupDir::with_rfc3339(&self.backup_type, &self.backup_id, backup_time)?;
>                  let files = list_backup_files(l2_fd, backup_time)?;
>  
> -                list.push(BackupInfo { backup_dir, files });
> +                let protected = backup_dir.is_protected(base_path.to_owned());
> +
> +                list.push(BackupInfo { backup_dir, files, protected });
>  
>                  Ok(())
>              },
> @@ -253,6 +255,17 @@ impl BackupDir {
>          relative_path
>      }
>  
> +    pub fn protected_file(&self, mut path: PathBuf) -> PathBuf {

Please also name the parameter `base_path`.

It's already weird because it's `BackupDir::relative_path`
(explicit 'relative' in the name) vs `BackupGroup::group_path` (which is
also relative, but doesn't explicitly say that it is).

I'm also not convinced this should take a `PathBuf`, I find `&Path` (or
even Cow<Path> or AsRef<Path>) makes a lot more sense here, and all our
callers will be cloning anyway.
(and in the remaining patches, the only places where the path is already
an owned PathBuf should in fact not actually have an owned PathBuf in
the first place, so I'd rather have that cleaned up where it comes
from...)

> +        path.push(self.relative_path());
> +        path.push(".protected");
> +        path
> +    }
> +
> +    pub fn is_protected(&self, base_path: PathBuf) -> bool {

same

> +        let path = self.protected_file(base_path);
> +        path.exists()
> +    }
> +
>      pub fn backup_time_to_string(backup_time: i64) -> Result<String, Error> {
>          // fixme: can this fail? (avoid unwrap)
>          Ok(proxmox_time::epoch_to_rfc3339_utc(backup_time)?)
> @@ -293,6 +306,8 @@ pub struct BackupInfo {
>      pub backup_dir: BackupDir,
>      /// List of data files
>      pub files: Vec<String>,
> +    /// Protection Status
> +    pub protected: bool,
>  }
>  
>  impl BackupInfo {