[PATCH proxmox-backup v4 12/14] pbs-config: add helper to check new datastore config sections

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

Factor out the datastore config check so this can be done independent
from the datastore creation, which is going to happen without holding
the datastore config lock but rather within the maintenance type
create.

While moving the code, unify it with other call sites to have all
checks performed in a central location.

The nesting check is now no longer performed within the datastore
creation task but rather early on and for datastore creations
combined with storage from disks as well.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-config/src/datastore.rs      | 45 +++++++++++++++++++++++++++++++-
 src/api2/config/datastore.rs     | 40 ++++------------------------
 src/api2/node/disks/directory.rs |  5 +---
 src/api2/node/disks/zfs.rs       |  5 +---
 4 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index 2a200b35e..de397302f 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -1,10 +1,11 @@
 use std::collections::HashMap;
+use std::path::Path;
 use std::sync::LazyLock;
 
 use anyhow::Error;
 
 use proxmox_product_config::replace_privileged_config;
-use proxmox_schema::{AllOfSchema, ApiType};
+use proxmox_schema::{AllOfSchema, ApiType, param_bail};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
 
 use pbs_api_types::{DATASTORE_SCHEMA, DataStoreConfig, DatastoreBackendConfig, DatastoreTuning};
@@ -132,3 +133,45 @@ pub fn datastore_backend_type(store: &str) -> Result<pbs_api_types::DatastoreBac
 
     Ok(parse_backend_config(&store_config)?.ty.unwrap_or_default())
 }
+
+/// Validate that the provided datastore config can be safely added as additional config section.
+pub fn validate_new_datastore_config(
+    store: &DataStoreConfig,
+    config: &SectionConfigData,
+    _config_lock: &BackupLockGuard,
+) -> Result<(), Error> {
+    if config.sections.contains_key(&store.name) {
+        param_bail!("name", "datastore '{}' already exists.", store.name);
+    }
+
+    let path = Path::new(&store.path);
+    if store.backing_device.is_none() {
+        if path.is_relative() {
+            param_bail!("path", "expected an absolute path, '{}' is not", store.path);
+        }
+        if path.parent().is_none() {
+            param_bail!("path", "cannot create datastore in root path",);
+        }
+        if store.gc_on_unmount.unwrap_or(false) {
+            param_bail!(
+                "gc-on-unmount",
+                "GC on unmount is only supported on removable datastores",
+            );
+        }
+    } else {
+        if path.is_absolute() {
+            param_bail!(
+                "path",
+                "expected a relative on-device path, '{}' is not",
+                store.path
+            );
+        }
+    }
+
+    let existing_stores = config.convert_to_typed_array("datastore")?;
+    if let Err(err) = store.ensure_not_nested(&existing_stores) {
+        param_bail!("path", err);
+    }
+
+    Ok(())
+}
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 74118de92..7505e8c78 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -1,7 +1,7 @@
 use std::path::{Path, PathBuf};
 
 use ::serde::{Deserialize, Serialize};
-use anyhow::{Context, Error, bail, format_err};
+use anyhow::{Context, Error, format_err};
 use serde_json::Value;
 use tracing::warn;
 
@@ -93,26 +93,17 @@ impl Drop for UnmountGuard {
 }
 
 pub(crate) fn do_create_datastore(
-    _lock: BackupLockGuard,
+    lock: BackupLockGuard,
     mut config: SectionConfigData,
     mut datastore: DataStoreConfig,
     reuse_datastore: bool,
     overwrite_in_use: bool,
 ) -> Result<(), Error> {
-    let path: PathBuf = datastore.absolute_path().into();
-
-    if path.parent().is_none() && datastore.backing_device.is_none() {
-        bail!("cannot create datastore in root path");
-    }
-
-    let existing_stores = config.convert_to_typed_array("datastore")?;
-    if let Err(err) = datastore.ensure_not_nested(&existing_stores) {
-        param_bail!("path", err);
-    }
+    pbs_config::datastore::validate_new_datastore_config(&datastore, &config, &lock)?;
 
     let unmount_guard = if datastore.backing_device.is_some() {
         do_mount_device(datastore.clone())?;
-        UnmountGuard::new(Some(path.clone()))
+        UnmountGuard::new(Some(datastore.absolute_path().into()))
     } else {
         UnmountGuard::new(None)
     };
@@ -182,28 +173,7 @@ pub fn create_datastore(
     let lock = pbs_config::datastore::lock_config()?;
 
     let (section_config, _digest) = pbs_config::datastore::config()?;
-
-    if section_config.sections.contains_key(&config.name) {
-        param_bail!("name", "datastore '{}' already exists.", config.name);
-    }
-
-    if config.backing_device.is_none() && !config.path.starts_with("/") {
-        param_bail!(
-            "path",
-            "expected an absolute path, '{}' is not",
-            config.path
-        );
-    }
-    if config.backing_device.is_some() && config.path.starts_with("/") {
-        param_bail!(
-            "path",
-            "expected a relative on-device path, '{}' is not",
-            config.path
-        );
-    }
-    if config.backing_device.is_none() && config.gc_on_unmount.unwrap_or(false) {
-        param_bail!("gc-on-unmount", "only supported on removable datastores",);
-    }
+    pbs_config::datastore::validate_new_datastore_config(&config, &section_config, &lock)?;
 
     let mut prune_job_config = None;
     if config.keep.keeps_something() || !has_prune_job(&config.name)? {
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 573e11744..9979355e5 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -248,10 +248,7 @@ pub fn create_datastore_disk(
                     serde_json::from_value(json!({ "name": name, "path": mount_point }))?
                 };
                 let (config, _digest) = pbs_config::datastore::config()?;
-
-                if config.sections.contains_key(&datastore.name) {
-                    bail!("datastore '{}' already exists.", datastore.name);
-                }
+                pbs_config::datastore::validate_new_datastore_config(&datastore, &config, &lock)?;
 
                 crate::api2::config::datastore::do_create_datastore(
                     lock, config, datastore, false, false,
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 7a62e3f70..0e8ce75b5 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -308,10 +308,7 @@ pub fn create_zpool(
                     serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
 
                 let (config, _digest) = pbs_config::datastore::config()?;
-
-                if config.sections.contains_key(&datastore.name) {
-                    bail!("datastore '{}' already exists.", datastore.name);
-                }
+                pbs_config::datastore::validate_new_datastore_config(&datastore, &config, &lock)?;
 
                 crate::api2::config::datastore::do_create_datastore(
                     lock, config, datastore, false, false,
-- 
2.47.3