* [pbs-devel] [PATCH proxmox-backup 1/2] api: datastore: make group notes path helper a DataStore method
2025-05-07 15:38 [pbs-devel] [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist Christian Ebner
@ 2025-05-07 15:38 ` Christian Ebner
2025-05-07 15:38 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #6358: remove group note file if present on group destroy Christian Ebner
2025-06-04 11:31 ` [pbs-devel] applied-series: [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Christian Ebner @ 2025-05-07 15:38 UTC (permalink / raw)
To: pbs-devel
Move and make the helper function to get a backup groups notes file
path a `DataStore` method instead. This allows it to be reused when
access to the notes path is required from the datastore itself.
Further, use the plural `notes` wording also in the helper to be
consistent with the rest of the codebase.
In preparation for correctly removing the notes file from the backup
group on destruction.
No functional changes intended.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/datastore.rs | 11 +++++++++++
src/api2/admin/datastore.rs | 26 +++++++-------------------
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index cbf78ecb6..91c7e76be 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -40,6 +40,8 @@ use crate::DataBlob;
static DATASTORE_MAP: LazyLock<Mutex<HashMap<String, Arc<DataStoreImpl>>>> =
LazyLock::new(|| Mutex::new(HashMap::new()));
+const GROUP_NOTES_FILE_NAME: &str = "notes";
+
/// checks if auth_id is owner, or, if owner is a token, if
/// auth_id is the user of the token
pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error> {
@@ -524,6 +526,15 @@ impl DataStore {
full_path
}
+ /// Returns the absolute path of a backup groups notes file
+ pub fn group_notes_path(
+ &self,
+ ns: &BackupNamespace,
+ group: &pbs_api_types::BackupGroup,
+ ) -> PathBuf {
+ self.group_path(ns, group).join(GROUP_NOTES_FILE_NAME)
+ }
+
/// Returns the absolute path for backup_dir
pub fn snapshot_path(
&self,
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 392494488..cc7e17a29 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -4,7 +4,7 @@ use std::collections::HashSet;
use std::ffi::OsStr;
use std::ops::Deref;
use std::os::unix::ffi::OsStrExt;
-use std::path::{Path, PathBuf};
+use std::path::Path;
use std::sync::Arc;
use anyhow::{bail, format_err, Context, Error};
@@ -77,18 +77,6 @@ use crate::backup::{
use crate::server::jobstate::{compute_schedule_status, Job, JobState};
-const GROUP_NOTES_FILE_NAME: &str = "notes";
-
-fn get_group_note_path(
- store: &DataStore,
- ns: &BackupNamespace,
- group: &pbs_api_types::BackupGroup,
-) -> PathBuf {
- let mut note_path = store.group_path(ns, group);
- note_path.push(GROUP_NOTES_FILE_NAME);
- note_path
-}
-
// helper to unify common sequence of checks:
// 1. check privs on NS (full or limited access)
// 2. load datastore
@@ -244,8 +232,8 @@ pub fn list_groups(
})
.to_owned();
- let note_path = get_group_note_path(&datastore, &ns, group.as_ref());
- let comment = file_read_firstline(note_path).ok();
+ let notes_path = datastore.group_notes_path(&ns, group.as_ref());
+ let comment = file_read_firstline(notes_path).ok();
group_info.push(GroupListItem {
backup: group.into(),
@@ -2053,8 +2041,8 @@ pub fn get_group_notes(
&backup_group,
)?;
- let note_path = get_group_note_path(&datastore, &ns, &backup_group);
- Ok(file_read_optional_string(note_path)?.unwrap_or_else(|| "".to_owned()))
+ let notes_path = datastore.group_notes_path(&ns, &backup_group);
+ Ok(file_read_optional_string(notes_path)?.unwrap_or_else(|| "".to_owned()))
}
#[api(
@@ -2101,8 +2089,8 @@ pub fn set_group_notes(
&backup_group,
)?;
- let note_path = get_group_note_path(&datastore, &ns, &backup_group);
- replace_file(note_path, notes.as_bytes(), CreateOptions::new(), false)?;
+ let notes_path = datastore.group_notes_path(&ns, &backup_group);
+ replace_file(notes_path, notes.as_bytes(), CreateOptions::new(), false)?;
Ok(())
}
--
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] 4+ messages in thread* [pbs-devel] [PATCH proxmox-backup 2/2] fix #6358: remove group note file if present on group destroy
2025-05-07 15:38 [pbs-devel] [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist Christian Ebner
2025-05-07 15:38 ` [pbs-devel] [PATCH proxmox-backup 1/2] api: datastore: make group notes path helper a DataStore method Christian Ebner
@ 2025-05-07 15:38 ` Christian Ebner
2025-06-04 11:31 ` [pbs-devel] applied-series: [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist Fabian Grünbichler
2 siblings, 0 replies; 4+ messages in thread
From: Christian Ebner @ 2025-05-07 15:38 UTC (permalink / raw)
To: pbs-devel
Removing the group directory when forgetting a backup group or
removing the final backup snapshot of a group did not take into
consideration a potentially present group note file, leading for it
to fail.
Further, since the owner file is removed before trying to remove the
(not empty) group directory, the group will not be usable anymore as
the owner check will fail as well.
To fix this, remove the backup group's note file first, if present
and only after that try to cleanup the rest.
Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6358
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/backup_info.rs | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index d4732fdd9..22b4eddf3 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -244,6 +244,13 @@ impl BackupGroup {
/// Helper function, assumes that no more snapshots are present in the group.
fn remove_group_dir(&self) -> Result<(), Error> {
+ let note_path = self.store.group_notes_path(&self.ns, &self.group);
+ if let Err(err) = std::fs::remove_file(¬e_path) {
+ if err.kind() != std::io::ErrorKind::NotFound {
+ bail!("removing the note file '{note_path:?}' failed - {err}")
+ }
+ }
+
let owner_path = self.store.owner_path(&self.ns, &self.group);
std::fs::remove_file(&owner_path).map_err(|err| {
--
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] 4+ messages in thread