[PATCH proxmox-backup v4 13/22] datastore: make operation non-optional in lookups

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

Based on the requested operation, the datastore might not be
available as e.g. it can be in a maintenance mode not allowing read
or write access, but lookup is just fine.

All callsides should however specify this, so make this non-optional.

Only pub callable exception remains DataStore::open_path(), as this
is used for an example and opens the datastore via the raw directory
path instead of relying on the PBS instance.

On the datastore itself it is kept as optional internal field due to
the raw access limitations and also since datastore cloning must not
fail on active operation update errors.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs       | 21 ++++-------
 pbs-datastore/src/snapshot_reader.rs |  2 +-
 src/api2/admin/datastore.rs          | 52 ++++++++++++++--------------
 src/api2/admin/namespace.rs          |  6 ++--
 src/api2/backup/mod.rs               |  2 +-
 src/api2/reader/mod.rs               |  2 +-
 src/api2/status/mod.rs               |  4 +--
 src/api2/tape/backup.rs              |  4 +--
 src/api2/tape/restore.rs             |  4 +--
 src/bin/proxmox-backup-proxy.rs      |  4 +--
 src/server/prune_job.rs              |  2 +-
 src/server/pull.rs                   |  4 +--
 src/server/push.rs                   |  2 +-
 src/server/verify_job.rs             |  2 +-
 14 files changed, 52 insertions(+), 59 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 56e8867c5..5a66830ba 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -498,10 +498,7 @@ impl DataStore {
         Ok(())
     }
 
-    pub fn lookup_datastore(
-        name: &str,
-        operation: Option<Operation>,
-    ) -> Result<Arc<DataStore>, Error> {
+    pub fn lookup_datastore(name: &str, operation: Operation) -> Result<Arc<DataStore>, Error> {
         // Avoid TOCTOU between checking maintenance mode and updating active operation counter, as
         // we use it to decide whether it is okay to delete the datastore.
         let _config_lock = pbs_config::datastore::lock_config()?;
@@ -511,7 +508,7 @@ impl DataStore {
         let config: DataStoreConfig = section_config.lookup("datastore", name)?;
 
         if let Some(maintenance_mode) = config.get_maintenance_mode() {
-            if let Err(error) = maintenance_mode.check(operation) {
+            if let Err(error) = maintenance_mode.check(Some(operation)) {
                 bail!("datastore '{name}' is unavailable: {error}");
             }
         }
@@ -529,12 +526,10 @@ impl DataStore {
         let chunk_store = if let Some(datastore) = &entry {
             // Re-use DataStoreImpl
             if datastore.config_generation == gen_num && gen_num.is_some() {
-                if let Some(operation) = operation {
-                    update_active_operations(name, operation, 1)?;
-                }
+                update_active_operations(name, operation, 1)?;
                 return Ok(Arc::new(Self {
                     inner: Arc::clone(datastore),
-                    operation,
+                    operation: Some(operation),
                 }));
             }
             Arc::clone(&datastore.chunk_store)
@@ -555,13 +550,11 @@ impl DataStore {
         let datastore = Arc::new(datastore);
         datastore_cache.insert(name.to_string(), datastore.clone());
 
-        if let Some(operation) = operation {
-            update_active_operations(name, operation, 1)?;
-        }
+        update_active_operations(name, operation, 1)?;
 
         Ok(Arc::new(Self {
             inner: datastore,
-            operation,
+            operation: Some(operation),
         }))
     }
 
@@ -587,7 +580,7 @@ impl DataStore {
         {
             // the datastore drop handler does the checking if tasks are running and clears the
             // cache entry, so we just have to trigger it here
-            let _ = DataStore::lookup_datastore(name, Some(Operation::Lookup));
+            let _ = DataStore::lookup_datastore(name, Operation::Lookup);
         }
 
         Ok(())
diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
index e4608ea56..231b1f493 100644
--- a/pbs-datastore/src/snapshot_reader.rs
+++ b/pbs-datastore/src/snapshot_reader.rs
@@ -164,7 +164,7 @@ impl<F: Fn(&[u8; 32]) -> bool> Iterator for SnapshotChunkIterator<'_, F> {
 
                         let datastore = DataStore::lookup_datastore(
                             self.snapshot_reader.datastore_name(),
-                            Some(Operation::Read),
+                            Operation::Read,
                         )?;
                         let order =
                             datastore.get_chunks_in_order(&*index, &self.skip_fn, |_| Ok(()))?;
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index f4133011c..ebbe699df 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -83,7 +83,7 @@ fn check_privs_and_load_store(
     auth_id: &Authid,
     full_access_privs: u64,
     partial_access_privs: u64,
-    operation: Option<Operation>,
+    operation: Operation,
     backup_group: &pbs_api_types::BackupGroup,
 ) -> Result<Arc<DataStore>, Error> {
     let limited = check_ns_privs_full(store, ns, auth_id, full_access_privs, partial_access_privs)?;
@@ -134,7 +134,7 @@ pub fn list_groups(
         PRIV_DATASTORE_BACKUP,
     )?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
 
     datastore
         .iter_backup_groups(ns.clone())? // FIXME: Namespaces and recursion parameters!
@@ -251,7 +251,7 @@ pub async fn delete_group(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_PRUNE,
-            Some(Operation::Write),
+            Operation::Write,
             &group,
         )?;
 
@@ -318,7 +318,7 @@ pub async fn list_snapshot_files(
             &auth_id,
             PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Read),
+            Operation::Read,
             &backup_dir.group,
         )?;
 
@@ -372,7 +372,7 @@ pub async fn delete_snapshot(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_PRUNE,
-            Some(Operation::Write),
+            Operation::Write,
             &backup_dir.group,
         )?;
 
@@ -467,7 +467,7 @@ unsafe fn list_snapshots_blocking(
         PRIV_DATASTORE_BACKUP,
     )?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
 
     // FIXME: filter also owner before collecting, for doing that nicely the owner should move into
     // backup group and provide an error free (Err -> None) accessor
@@ -601,7 +601,7 @@ pub async fn status(
         }
     };
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
 
     let (counts, gc_status) = if verbose {
         let filter_owner = if store_privs & PRIV_DATASTORE_AUDIT != 0 {
@@ -728,7 +728,7 @@ pub fn verify(
         PRIV_DATASTORE_BACKUP,
     )?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
     let ignore_verified = ignore_verified.unwrap_or(true);
 
     let worker_id;
@@ -908,7 +908,7 @@ pub fn prune(
         &auth_id,
         PRIV_DATASTORE_MODIFY,
         PRIV_DATASTORE_PRUNE,
-        Some(Operation::Write),
+        Operation::Write,
         &group,
     )?;
 
@@ -1080,7 +1080,7 @@ pub fn prune_datastore(
         true,
     )?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
     let ns = prune_options.ns.clone().unwrap_or_default();
     let worker_id = format!("{store}:{ns}");
 
@@ -1118,7 +1118,7 @@ pub fn start_garbage_collection(
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let job = Job::new("garbage_collection", &store)
@@ -1165,7 +1165,7 @@ pub fn garbage_collection_status(
         ..Default::default()
     };
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
     let status_in_memory = datastore.last_gc_status();
     let state_file = JobState::load("garbage_collection", &store)
         .map_err(|err| log::error!("could not open GC statefile for {store}: {err}"))
@@ -1311,7 +1311,7 @@ pub fn download_file(
             &auth_id,
             PRIV_DATASTORE_READ,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Read),
+            Operation::Read,
             &backup_dir.group,
         )?;
 
@@ -1396,7 +1396,7 @@ pub fn download_file_decoded(
             &auth_id,
             PRIV_DATASTORE_READ,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Read),
+            Operation::Read,
             &backup_dir_api.group,
         )?;
 
@@ -1525,7 +1525,7 @@ pub fn upload_backup_log(
             &auth_id,
             0,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Write),
+            Operation::Write,
             &backup_dir_api.group,
         )?;
         let backup_dir = datastore.backup_dir(backup_ns.clone(), backup_dir_api.clone())?;
@@ -1623,7 +1623,7 @@ pub async fn catalog(
         &auth_id,
         PRIV_DATASTORE_READ,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Read),
+        Operation::Read,
         &backup_dir.group,
     )?;
 
@@ -1745,7 +1745,7 @@ pub fn pxar_file_download(
             &auth_id,
             PRIV_DATASTORE_READ,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Read),
+            Operation::Read,
             &backup_dir.group,
         )?;
 
@@ -1877,7 +1877,7 @@ pub fn get_rrd_stats(
     cf: RrdMode,
     _param: Value,
 ) -> Result<Value, Error> {
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
     let disk_manager = crate::tools::disks::DiskManage::new();
 
     let mut rrd_fields = vec![
@@ -1967,7 +1967,7 @@ pub fn get_group_notes(
         &auth_id,
         PRIV_DATASTORE_AUDIT,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Read),
+        Operation::Read,
         &backup_group,
     )?;
 
@@ -2015,7 +2015,7 @@ pub fn set_group_notes(
         &auth_id,
         PRIV_DATASTORE_MODIFY,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Write),
+        Operation::Write,
         &backup_group,
     )?;
 
@@ -2062,7 +2062,7 @@ pub fn get_notes(
         &auth_id,
         PRIV_DATASTORE_AUDIT,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Read),
+        Operation::Read,
         &backup_dir.group,
     )?;
 
@@ -2115,7 +2115,7 @@ pub fn set_notes(
         &auth_id,
         PRIV_DATASTORE_MODIFY,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Write),
+        Operation::Write,
         &backup_dir.group,
     )?;
 
@@ -2160,7 +2160,7 @@ pub fn get_protection(
         &auth_id,
         PRIV_DATASTORE_AUDIT,
         PRIV_DATASTORE_BACKUP,
-        Some(Operation::Read),
+        Operation::Read,
         &backup_dir.group,
     )?;
 
@@ -2210,7 +2210,7 @@ pub async fn set_protection(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_BACKUP,
-            Some(Operation::Write),
+            Operation::Write,
             &backup_dir.group,
         )?;
 
@@ -2264,7 +2264,7 @@ pub async fn set_backup_owner(
             PRIV_DATASTORE_BACKUP,
         )?;
 
-        let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+        let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
 
         let backup_group = datastore.backup_group(ns, backup_group);
         let owner = backup_group.get_owner()?;
@@ -2749,7 +2749,7 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
 /// Performs an s3 refresh for given datastore. Expects the store to already be in maintenance mode
 /// s3-refresh.
 pub(crate) fn do_s3_refresh(store: &str, worker: &dyn WorkerTaskContext) -> Result<(), Error> {
-    let datastore = DataStore::lookup_datastore(store, Some(Operation::Lookup))?;
+    let datastore = DataStore::lookup_datastore(store, Operation::Lookup)?;
     run_maintenance_locked(store, MaintenanceType::S3Refresh, worker, || {
         proxmox_async::runtime::block_on(datastore.s3_refresh())
     })
diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index 6cf88d89e..30e24d8db 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -54,7 +54,7 @@ pub fn create_namespace(
 
     check_ns_modification_privs(&store, &ns, &auth_id)?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
 
     datastore.create_namespace(&parent, name)
 }
@@ -97,7 +97,7 @@ pub fn list_namespaces(
     // get result up-front to avoid cloning NS, it's relatively cheap anyway (no IO normally)
     let parent_access = check_ns_privs(&store, &parent, &auth_id, NS_PRIVS_OK);
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
 
     let iter = match datastore.recursive_iter_backup_ns_ok(parent, max_depth) {
         Ok(iter) => iter,
@@ -162,7 +162,7 @@ pub fn delete_namespace(
 
     check_ns_modification_privs(&store, &ns, &auth_id)?;
 
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
 
     let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups)?;
     if !removed_all {
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 3e6b7a950..946510e85 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -99,7 +99,7 @@ fn upgrade_to_backup_protocol(
             )
             .map_err(|err| http_err!(FORBIDDEN, "{err}"))?;
 
-        let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+        let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
 
         let protocols = parts
             .headers
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index f7adc366f..9262eb6cb 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -96,7 +96,7 @@ fn upgrade_to_backup_reader_protocol(
             bail!("no permissions on /{}", acl_path.join("/"));
         }
 
-        let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+        let datastore = DataStore::lookup_datastore(&store, Operation::Read)?;
 
         let backup_dir = pbs_api_types::BackupDir::deserialize(&param)?;
 
diff --git a/src/api2/status/mod.rs b/src/api2/status/mod.rs
index 605072d60..885fdb0cc 100644
--- a/src/api2/status/mod.rs
+++ b/src/api2/status/mod.rs
@@ -69,7 +69,7 @@ pub async fn datastore_status(
         };
 
         if !allowed {
-            if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
+            if let Ok(datastore) = DataStore::lookup_datastore(store, Operation::Lookup) {
                 if can_access_any_namespace(datastore, &auth_id, &user_info) {
                     list.push(DataStoreStatusListItem::empty(store, None, mount_status));
                 }
@@ -77,7 +77,7 @@ pub async fn datastore_status(
             continue;
         }
 
-        let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
+        let datastore = match DataStore::lookup_datastore(store, Operation::Read) {
             Ok(datastore) => datastore,
             Err(err) => {
                 list.push(DataStoreStatusListItem::empty(
diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index 16a26b83e..47e8d0209 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -152,7 +152,7 @@ pub fn do_tape_backup_job(
 
     let worker_type = job.jobtype().to_string();
 
-    let datastore = DataStore::lookup_datastore(&setup.store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&setup.store, Operation::Read)?;
 
     let (config, _digest) = pbs_config::media_pool::config()?;
     let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
@@ -310,7 +310,7 @@ pub fn backup(
 
     check_backup_permission(&auth_id, &setup.store, &setup.pool, &setup.drive)?;
 
-    let datastore = DataStore::lookup_datastore(&setup.store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&setup.store, Operation::Read)?;
 
     let (config, _digest) = pbs_config::media_pool::config()?;
     let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 4f2ee3db6..92529a76d 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -144,10 +144,10 @@ impl TryFrom<String> for DataStoreMap {
             if let Some(index) = store.find('=') {
                 let mut target = store.split_off(index);
                 target.remove(0); // remove '='
-                let datastore = DataStore::lookup_datastore(&target, Some(Operation::Write))?;
+                let datastore = DataStore::lookup_datastore(&target, Operation::Write)?;
                 map.insert(store, datastore);
             } else if default.is_none() {
-                default = Some(DataStore::lookup_datastore(&store, Some(Operation::Write))?);
+                default = Some(DataStore::lookup_datastore(&store, Operation::Write)?);
             } else {
                 bail!("multiple default stores given");
             }
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 9f14b70df..0b2552c32 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -545,7 +545,7 @@ async fn schedule_datastore_garbage_collection() {
 
         {
             // limit datastore scope due to Op::Lookup
-            let datastore = match DataStore::lookup_datastore(&store, Some(Operation::Lookup)) {
+            let datastore = match DataStore::lookup_datastore(&store, Operation::Lookup) {
                 Ok(datastore) => datastore,
                 Err(err) => {
                     eprintln!("lookup_datastore failed - {err}");
@@ -588,7 +588,7 @@ async fn schedule_datastore_garbage_collection() {
             Err(_) => continue, // could not get lock
         };
 
-        let datastore = match DataStore::lookup_datastore(&store, Some(Operation::Write)) {
+        let datastore = match DataStore::lookup_datastore(&store, Operation::Write) {
             Ok(datastore) => datastore,
             Err(err) => {
                 log::warn!("skipping scheduled GC on {store}, could look it up - {err}");
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index 9d07558d3..bb86a323e 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -133,7 +133,7 @@ pub fn do_prune_job(
     auth_id: &Authid,
     schedule: Option<String>,
 ) -> Result<String, Error> {
-    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
 
     let worker_type = job.jobtype().to_string();
     let auth_id = auth_id.clone();
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 15d8b9deb..412a59e66 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -113,11 +113,11 @@ impl PullParameters {
             })
         } else {
             Arc::new(LocalSource {
-                store: DataStore::lookup_datastore(remote_store, Some(Operation::Read))?,
+                store: DataStore::lookup_datastore(remote_store, Operation::Read)?,
                 ns: remote_ns,
             })
         };
-        let store = DataStore::lookup_datastore(store, Some(Operation::Write))?;
+        let store = DataStore::lookup_datastore(store, Operation::Write)?;
         let backend = store.backend()?;
         let target = PullTarget { store, ns, backend };
 
diff --git a/src/server/push.rs b/src/server/push.rs
index d7884fce2..92bbbb9fc 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -109,7 +109,7 @@ impl PushParameters {
         let remove_vanished = remove_vanished.unwrap_or(false);
         let encrypted_only = encrypted_only.unwrap_or(false);
         let verified_only = verified_only.unwrap_or(false);
-        let store = DataStore::lookup_datastore(store, Some(Operation::Read))?;
+        let store = DataStore::lookup_datastore(store, Operation::Read)?;
 
         if !store.namespace_exists(&ns) {
             bail!(
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index e0b03155c..2ec8c5138 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -15,7 +15,7 @@ pub fn do_verification_job(
     schedule: Option<String>,
     to_stdout: bool,
 ) -> Result<String, Error> {
-    let datastore = DataStore::lookup_datastore(&verification_job.store, Some(Operation::Read))?;
+    let datastore = DataStore::lookup_datastore(&verification_job.store, Operation::Read)?;
 
     let outdated_after = verification_job.outdated_after;
     let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
-- 
2.47.3