[pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking

[pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking

[Dominik Csapak]

used_datastores returned the 'target', but in the full_restore_worker,
we interpreted it as the source and searched for a mapping
(which we then locked)

since we cannot return a HashSet of Arc<T> (missing Hash trait on DataStore),
we have now a map of source -> target

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/restore.rs | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 93803f14..f24ae23c 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -88,17 +88,17 @@ impl TryFrom<String> for DataStoreMap {
 }
 
 impl DataStoreMap {
-    fn used_datastores<'a>(&self) -> HashSet<&str> {
-        let mut set = HashSet::new();
-        for store in self.map.values() {
-            set.insert(store.name());
+    fn used_datastores<'a>(&self) -> HashMap<&str, Arc<DataStore>> {
+        let mut map = HashMap::new();
+        for (source, target) in self.map.iter() {
+            map.insert(source.as_str(), Arc::clone(target));
         }
 
         if let Some(ref store) = self.default {
-            set.insert(store.name());
+            map.insert("", Arc::clone(store));
         }
 
-        set
+        map
     }
 
     fn get_datastore(&self, source: &str) -> Option<Arc<DataStore>> {
@@ -200,8 +200,8 @@ pub fn restore(
         bail!("no datastores given");
     }
 
-    for store in used_datastores.iter() {
-        check_datastore_privs(&user_info, store, &auth_id, &owner)?;
+    for (_, target) in used_datastores.iter() {
+        check_datastore_privs(&user_info, target.name(), &auth_id, &owner)?;
     }
 
     let privs = user_info.lookup_privs(&auth_id, &["tape", "drive", &drive]);
@@ -233,7 +233,7 @@ pub fn restore(
 
     let taskid = used_datastores
         .iter()
-        .map(|s| s.to_string())
+        .map(|(_, t)| t.name().to_string())
         .collect::<Vec<String>>()
         .join(", ");
 
@@ -349,7 +349,7 @@ fn restore_full_worker(
         store_map
             .used_datastores()
             .into_iter()
-            .map(String::from)
+            .map(|(_, t)| String::from(t.name()))
             .collect::<Vec<String>>()
             .join(", "),
     );
@@ -366,12 +366,10 @@ fn restore_full_worker(
     );
 
     let mut datastore_locks = Vec::new();
-    for store_name in store_map.used_datastores() {
+    for (_, target) in store_map.used_datastores() {
         // explicit create shared lock to prevent GC on newly created chunks
-        if let Some(store) = store_map.get_datastore(store_name) {
-            let shared_store_lock = store.try_shared_chunk_store_lock()?;
-            datastore_locks.push(shared_store_lock);
-        }
+        let shared_store_lock = target.try_shared_chunk_store_lock()?;
+        datastore_locks.push(shared_store_lock);
     }
 
     let mut checked_chunks_map = HashMap::new();
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/2] api: tape/restore: skip snapshot if owner check failed

[Dominik Csapak]

instead of aborting the whole restore

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/restore.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index f24ae23c..a7ed119a 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -433,12 +433,14 @@ fn restore_list_worker(
                 datastore.create_locked_backup_group(backup_dir.as_ref(), restore_owner)?;
             if restore_owner != &owner {
                 // only the owner is allowed to create additional snapshots
-                bail!(
+                task_warn!(
+                    worker,
                     "restore '{}' failed - owner check failed ({} != {})",
                     snapshot,
                     restore_owner,
                     owner
                 );
+                continue;
             }
 
             let (media_id, file_num) = if let Some((media_uuid, file_num)) =
-- 
2.30.2

[pbs-devel] applied-series: Re: [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking

[Thomas Lamprecht]

On 09/05/2022 12:41, Dominik Csapak wrote:
> used_datastores returned the 'target', but in the full_restore_worker,
> we interpreted it as the source and searched for a mapping
> (which we then locked)
> 
> since we cannot return a HashSet of Arc<T> (missing Hash trait on DataStore),
> we have now a map of source -> target
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/tape/restore.rs | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
>

applied both patches, thanks!