From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id D45D71FF16B
for <inbox@lore.proxmox.com>; Thu, 20 Mar 2025 13:12:18 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CC500298;
Thu, 20 Mar 2025 13:12:17 +0100 (CET)
From: Hannes Laimer <h.laimer@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Thu, 20 Mar 2025 13:11:25 +0100
Message-Id: <20250320121125.114333-1-h.laimer@proxmox.com>
X-Mailer: git-send-email 2.39.5
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.025 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [datastore.rs]
Subject: [pbs-devel] [PATCH proxmox-backup v2] api: config: use guard for
unmounting on failed datastore creation
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
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>
---
v2, thanks @Wolfgang:
- replace `should_unmount` flag with wrapping path in an Option
src/api2/config/datastore.rs | 82 +++++++++++++++++++++---------------
1 file changed, 48 insertions(+), 34 deletions(-)
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index fe3260f6d..58acaa861 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,29 @@ pub fn list_datastores(
Ok(list.into_iter().filter(filter_by_privs).collect())
}
+struct UnmountGuard {
+ path: Option<PathBuf>,
+}
+
+impl UnmountGuard {
+ fn new(path: Option<PathBuf>) -> Self {
+ UnmountGuard { path }
+ }
+ fn disable(mut self) {
+ self.path = None;
+ }
+}
+
+impl Drop for UnmountGuard {
+ fn drop(&mut self) {
+ if let Some(path) = &self.path {
+ if let Err(e) = unmount_by_mountpoint(path) {
+ warn!("could not unmount device: {e}");
+ }
+ }
+ }
+}
+
pub(crate) fn do_create_datastore(
_lock: BackupLockGuard,
mut config: SectionConfigData,
@@ -87,59 +110,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 unmount_guard = if datastore.backing_device.is_some() {
+ do_mount_device(datastore.clone())?;
+ UnmountGuard::new(Some(path.clone()))
+ } else {
+ UnmountGuard::new(None)
+ };
+
+ 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