Re: [pbs-devel] [PATCH proxmox-backup v3 08/13] api2/admin/datastore: add get/set_protection

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

1 error handling issue, and another nit

On Wed, Oct 27, 2021 at 01:22:33PM +0200, Dominik Csapak wrote:
> for gettin/setting the protected flag for snapshots (akin to notes)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 24 +++++++++
>  src/api2/admin/datastore.rs    | 92 ++++++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index e32887de..5049cb3d 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -858,6 +858,30 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    /// Updates the protection status of the specified snapshot.
> +    pub fn update_protection(
> +        &self,
> +        backup_dir: &BackupDir,
> +        protection: bool
> +    ) -> Result<(), Error> {
> +        let full_path = self.snapshot_path(backup_dir);
> +
> +        let _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?;
> +
> +        let protected_path = backup_dir.protected_file(self.base_path());
> +        if protection {
> +            std::fs::File::create(protected_path)
> +                .map_err(|err| format_err!("could not create protection file: {}", err))?;

Should we not also ignore `ErrorKind::AlreadyExists` here?
After all, we ignore `NotFound` when removing it.

> +        } else if let Err(err) = std::fs::remove_file(protected_path) {
> +            // ignore error for non-existing file
> +            if err.kind() != std::io::ErrorKind::NotFound {
> +                bail!("could not remove protection file: {}", err);
> +            }
> +        }
> +
> +        Ok(())
> +    }
> +
>      pub fn verify_new(&self) -> bool {
>          self.verify_new
>      }
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index fe962e4e..b9bcde12 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1750,6 +1750,92 @@ pub fn set_notes(
>      Ok(())
>  }
>  
> +#[api(
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            "backup-type": {
> +                schema: BACKUP_TYPE_SCHEMA,
> +            },
> +            "backup-id": {
> +                schema: BACKUP_ID_SCHEMA,
> +            },
> +            "backup-time": {
> +                schema: BACKUP_TIME_SCHEMA,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
> +    },
> +)]
> +/// Query protection for a specific backup
> +pub fn get_protection(
> +    store: String,
> +    backup_type: String,
> +    backup_id: String,
> +    backup_time: i64,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<bool, Error> {
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> +
> +    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_AUDIT)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +
> +    Ok(protected_path.exists())

since you don't need the path, use `.is_protected()`

> +}
> +