Re: [pbs-devel] [PATCH proxmox-backup v7 2/6] datastore: add check for maintenance in lookup

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

On Fri, Feb 04, 2022 at 11:17:25AM +0000, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs       | 19 ++++++++---
>  pbs-datastore/src/snapshot_reader.rs |  6 +++-
>  src/api2/admin/datastore.rs          | 50 ++++++++++++++--------------
>  src/api2/backup/mod.rs               |  4 +--
>  src/api2/reader/mod.rs               |  6 ++--
>  src/api2/status.rs                   |  4 +--
>  src/api2/tape/backup.rs              |  6 ++--
>  src/api2/tape/restore.rs             |  6 ++--
>  src/bin/proxmox-backup-proxy.rs      |  4 +--
>  src/server/prune_job.rs              |  4 +--
>  src/server/pull.rs                   |  4 +--
>  src/server/verify_job.rs             |  4 +--
>  12 files changed, 66 insertions(+), 51 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 37ef753e..c65d4574 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -15,7 +15,9 @@ use proxmox_sys::WorkerTaskContext;
>  use proxmox_sys::{task_log, task_warn};
>  use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
>  
> -use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus, HumanByte};
> +use pbs_api_types::{
> +    UPID, DataStoreConfig, Authid, MaintenanceType, Operation, GarbageCollectionStatus, HumanByte
> +};
>  use pbs_config::{open_backup_lockfile, BackupLockGuard};
>  
>  use crate::DataBlob;
> @@ -60,13 +62,22 @@ pub struct DataStore {
>  }
>  
>  impl DataStore {
> -
> -    pub fn lookup_datastore(name: &str) -> Result<Arc<DataStore>, Error> {
> -
> +    pub fn lookup_datastore(
> +        name: &str,
> +        operation: Option<Operation>,
> +    ) -> Result<Arc<DataStore>, Error> {
>          let (config, _digest) = pbs_config::datastore::config()?;
>          let config: DataStoreConfig = config.lookup("datastore", name)?;
>          let path = PathBuf::from(&config.path);
>  
> +        match (&config.maintenance_type, operation) {
> +            (Some(MaintenanceType::ReadOnly(message)), Some(Operation::Write))
> +            | (Some(MaintenanceType::Offline(message)), Some(_)) => {
> +                bail!("Datastore '{}' is in maintenance mode: {}", name, message);

I'd like to see the current-confg vs requested operation check logic
as a method of the `MaintenanceType` actually. Even if we only use it
once for now, I think the code will be much nicer this way.

This could one of:

    impl MaintenanceType {
        fn check(current: &Option<Self>, other: &Option<Self>) -> Result<(), Error>;
        fn check(&self, other: &Option<Self>) -> Result<(), Error>;
    }

resulting in either:

    MaintenanceType::check(&config.maintenance_type, operation)

or

    if let Some(cur) = config.maintenance_type.clone() {
        cur.check(&operation)?;
    }

both are also easier on the eyes than a multi-line match pattern ;-)