[pbs-devel] [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist

[pbs-devel] [PATCH proxmox-backup 0/2] fix #6358: group removal fails if group notes exist

[Christian Ebner]

These patches fix an issue with the backup group removal failing and
leaving behind an unusable backup group, due to not taking a possibly
present backup group's notes file into account.

The series consists of 2 patches, the first being preparatory,
allowing to reuse the group notes filepath helper, the second fixing
the actual issue by conditionally removing the notes file before
further group directory cleanup.

Link to the bugtracker issue:
https://bugzilla.proxmox.com/show_bug.cgi?id=6358

Christian Ebner (2):
  api: datastore: make group notes path helper a DataStore method
  fix #6358: remove group note file if present on group destroy

 pbs-datastore/src/backup_info.rs |  7 +++++++
 pbs-datastore/src/datastore.rs   | 11 +++++++++++
 src/api2/admin/datastore.rs      | 26 +++++++-------------------
 3 files changed, 25 insertions(+), 19 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 1/2] api: datastore: make group notes path helper a DataStore method

[Christian Ebner]

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

[pbs-devel] [PATCH proxmox-backup 2/2] fix #6358: remove group note file if present on group destroy

[Christian Ebner]

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(&note_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