[PATCH proxmox-backup v4 3/5] pbs-datastore: pass expected files to cleanup_unreferenced_files

[Jakob Klocker <j.klocker@proxmox.com>]

Instead of passing the whole manifest and building the set of wanted
files inside cleanup_unreferenced_files, let the caller pass the set
directly. Add a BackupManifest::expected_files helper returning the set
of all files which might be encountered in the snapshot described by
the manifest, i.e. the manifest entries plus the manifest and client
log blobs.

This decouples the cleanup from the manifest instance and is in
preparation for writing a modified manifest during pull, without
having to keep the manifest around until the cleanup step.

No functional change intended.

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 24 ++++++++++++------------
 pbs-datastore/src/manifest.rs    | 19 ++++++++++++++++++-
 src/server/pull.rs               |  3 ++-
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index be4ec8b3e..cce2b699f 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::fmt;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::os::unix::prelude::OsStrExt;
@@ -16,8 +17,7 @@ use proxmox_systemd::escape_unit;
 
 use pbs_api_types::{
     ArchiveType, Authid, BACKUP_DATE_REGEX, BackupArchiveName, BackupGroupDeleteStats,
-    BackupNamespace, BackupType, CLIENT_LOG_BLOB_NAME, GroupFilter, MANIFEST_BLOB_NAME,
-    VerifyState,
+    BackupNamespace, BackupType, GroupFilter, MANIFEST_BLOB_NAME, VerifyState,
 };
 use pbs_config::{BackupLockGuard, open_backup_lockfile};
 
@@ -1029,17 +1029,17 @@ impl BackupDir {
         Ok(())
     }
 
-    /// Cleans up the backup directory by removing any file not mentioned in the manifest.
-    pub fn cleanup_unreferenced_files(&self, manifest: &BackupManifest) -> Result<(), Error> {
+    /// Removes any file in the backup directory not present in `files_to_keep`.
+    ///
+    /// The set is supplied by the caller (see `BackupManifest::expected_files`)
+    /// rather than derived here, so passing an incomplete set will delete files
+    /// that should be retained.
+    pub fn cleanup_unreferenced_files(
+        &self,
+        files_to_keep: &HashSet<BackupArchiveName>,
+    ) -> Result<(), Error> {
         let full_path = self.full_path();
 
-        let mut wanted_files = std::collections::HashSet::new();
-        wanted_files.insert(MANIFEST_BLOB_NAME.to_string());
-        wanted_files.insert(CLIENT_LOG_BLOB_NAME.to_string());
-        manifest.files().iter().for_each(|item| {
-            wanted_files.insert(item.filename.to_string());
-        });
-
         for item in proxmox_sys::fs::read_subdir(libc::AT_FDCWD, &full_path)?.flatten() {
             if let Some(file_type) = item.file_type() {
                 if file_type != nix::dir::Type::File {
@@ -1051,7 +1051,7 @@ impl BackupDir {
                 continue;
             };
             if let Ok(name) = std::str::from_utf8(file_name) {
-                if wanted_files.contains(name) {
+                if files_to_keep.contains(name) {
                     continue;
                 }
             }
diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 4d482c252..8c7a5f8b6 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -1,9 +1,14 @@
+use std::collections::HashSet;
+
 use anyhow::{Error, bail, format_err};
 
 use serde::{Deserialize, Serialize};
 use serde_json::{Value, json};
 
-use pbs_api_types::{BackupArchiveName, BackupType, CryptMode, Fingerprint, SnapshotVerifyState};
+use pbs_api_types::{
+    BackupArchiveName, BackupType, CLIENT_LOG_BLOB_NAME, CryptMode, Fingerprint,
+    MANIFEST_BLOB_NAME, SnapshotVerifyState,
+};
 use pbs_tools::crypt_config::CryptConfig;
 
 use super::DataBlob;
@@ -295,6 +300,18 @@ impl BackupManifest {
 
         Ok(Some(Deserialize::deserialize(value)?))
     }
+
+    /// Returns the set of all files which might be encountered in
+    /// the snapshot represented by given manifest.
+    pub fn expected_files(&self) -> HashSet<BackupArchiveName> {
+        let mut expected = HashSet::new();
+        expected.insert(MANIFEST_BLOB_NAME.clone());
+        expected.insert(CLIENT_LOG_BLOB_NAME.clone());
+        self.files().iter().for_each(|item| {
+            expected.insert(item.filename.clone());
+        });
+        expected
+    }
 }
 
 impl TryFrom<DataBlob> for BackupManifest {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 1e24b038b..ef7f714b7 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -852,6 +852,7 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
+    let files_to_keep = manifest.expected_files();
     if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
@@ -914,7 +915,7 @@ async fn pull_snapshot<'a>(
     fetch_log(crypt_config).await?;
 
     let snapshot = snapshot.clone();
-    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
+    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&files_to_keep))
         .await?
         .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
 
-- 
2.47.3