[pbs-devel] [PATCH proxmox-backup] api: config: use guard for unmounting on failed datastore creation

* [pbs-devel] [PATCH proxmox-backup] api: config: use guard for unmounting on failed datastore creation
@ 2025-03-20  9:48 Hannes Laimer
  2025-03-20 11:57 ` Wolfgang Bumiller
  0 siblings, 1 reply; 2+ messages in thread
From: Hannes Laimer @ 2025-03-20  9:48 UTC (permalink / raw)
  To: pbs-devel

Currently if any `?`/`bail!` happens between mounting and completing
the creation process unmounting will be skipped. Adding this guard
solves that problem and makes it easier to add things in the future
without having to worry about a disk not being unmounted in case of a
failed creation.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
Reported-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 src/api2/config/datastore.rs | 86 ++++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 34 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index fe3260f6d..99c699814 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::{bail, format_err, Error};
+use anyhow::{bail, Error};
 use hex::FromHex;
 use serde_json::Value;
 use tracing::warn;
@@ -70,6 +70,33 @@ pub fn list_datastores(
     Ok(list.into_iter().filter(filter_by_privs).collect())
 }
 
+struct UnmountGuard {
+    path: PathBuf,
+    should_unmount: bool,
+}
+
+impl UnmountGuard {
+    fn new(path: PathBuf, should_unmount: bool) -> Self {
+        UnmountGuard {
+            path,
+            should_unmount,
+        }
+    }
+    fn disable(&mut self) {
+        self.should_unmount = false;
+    }
+}
+
+impl Drop for UnmountGuard {
+    fn drop(&mut self) {
+        if self.should_unmount {
+            if let Err(e) = unmount_by_mountpoint(&self.path) {
+                warn!("could not unmount device: {e}");
+            }
+        }
+    }
+}
+
 pub(crate) fn do_create_datastore(
     _lock: BackupLockGuard,
     mut config: SectionConfigData,
@@ -87,59 +114,50 @@ pub(crate) fn do_create_datastore(
         param_bail!("path", err);
     }
 
-    let need_unmount = datastore.backing_device.is_some();
-    if need_unmount {
-        do_mount_device(datastore.clone())?;
-    };
-
     let tuning: DatastoreTuning = serde_json::from_value(
         DatastoreTuning::API_SCHEMA
             .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
     )?;
 
-    let res = if reuse_datastore {
-        ChunkStore::verify_chunkstore(&path)
+    let mut unmount_guard = if datastore.backing_device.is_some() {
+        do_mount_device(datastore.clone())?;
+        UnmountGuard::new(path.clone(), true)
+    } else {
+        UnmountGuard::new(path.clone(), false)
+    };
+
+    if reuse_datastore {
+        ChunkStore::verify_chunkstore(&path)?;
     } else {
-        let mut is_empty = true;
         if let Ok(dir) = std::fs::read_dir(&path) {
             for file in dir {
                 let name = file?.file_name();
                 let name = name.to_str();
                 if !name.is_some_and(|name| name.starts_with('.') || name == "lost+found") {
-                    is_empty = false;
-                    break;
+                    bail!("datastore path not empty");
                 }
             }
         }
-        if is_empty {
-            let backup_user = pbs_config::backup_user()?;
-            ChunkStore::create(
-                &datastore.name,
-                path.clone(),
-                backup_user.uid,
-                backup_user.gid,
-                tuning.sync_level.unwrap_or_default(),
-            )
-            .map(|_| ())
-        } else {
-            Err(format_err!("datastore path not empty"))
-        }
+        let backup_user = pbs_config::backup_user()?;
+        ChunkStore::create(
+            &datastore.name,
+            path.clone(),
+            backup_user.uid,
+            backup_user.gid,
+            tuning.sync_level.unwrap_or_default(),
+        )
+        .map(|_| ())?;
     };
 
-    if res.is_err() {
-        if need_unmount {
-            if let Err(e) = unmount_by_mountpoint(&path) {
-                warn!("could not unmount device: {e}");
-            }
-        }
-        return res;
-    }
-
     config.set_data(&datastore.name, "datastore", &datastore)?;
 
     pbs_config::datastore::save_config(&config)?;
 
-    jobstate::create_state_file("garbage_collection", &datastore.name)
+    jobstate::create_state_file("garbage_collection", &datastore.name)?;
+
+    unmount_guard.disable();
+
+    Ok(())
 }
 
 #[api(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread