Merge pull request #3674 from mulkieran/destroy-filesystems-set

Enforce set invariant in destroy_filesystems
stratis-storage · Aug 15, 2024 · a5cdc20 · a5cdc20
2 parents eafccd0 + d4c4e7b
commit a5cdc20
Show file tree

Hide file tree

Showing 9 changed files with 42 additions and 27 deletions.
diff --git a/src/dbus_api/pool/pool_3_0/methods.rs b/src/dbus_api/pool/pool_3_0/methods.rs
@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use dbus::{arg::Array, Message};
 use dbus_tree::{MTSync, MethodInfo, MethodResult};
@@ -168,7 +168,7 @@ pub fn destroy_filesystems(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodRe
     let result = handle_action!(
         pool.destroy_filesystems(
             &pool_name,
-            &filesystem_map.keys().cloned().collect::<Vec<_>>(),
+            &filesystem_map.keys().cloned().collect::<HashSet<_>>(),
         ),
         dbus_context,
         pool_path.get_name()

diff --git a/src/dbus_api/pool/pool_3_7/methods.rs b/src/dbus_api/pool/pool_3_7/methods.rs
@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use dbus::{arg::Array, Message};
 use dbus_tree::{MTSync, MethodInfo, MethodResult};
@@ -56,7 +56,7 @@ pub fn destroy_filesystems(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodRe
     let result = handle_action!(
         pool.destroy_filesystems(
             &pool_name,
-            &filesystem_map.keys().cloned().collect::<Vec<_>>(),
+            &filesystem_map.keys().cloned().collect::<HashSet<_>>(),
         ),
         dbus_context,
         pool_path.get_name()

diff --git a/src/engine/engine.rs b/src/engine/engine.rs
@@ -209,7 +209,7 @@ pub trait Pool: Debug + Send + Sync {
     fn destroy_filesystems(
         &mut self,
         pool_name: &str,
-        fs_uuids: &[FilesystemUuid],
+        fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>>;
 
     /// Rename filesystem

diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs
@@ -487,7 +487,7 @@ impl Pool for SimPool {
     fn destroy_filesystems(
         &mut self,
         _pool_name: &str,
-        fs_uuids: &[FilesystemUuid],
+        fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
         let mut removed = Vec::new();
         for &uuid in fs_uuids {
@@ -909,7 +909,7 @@ mod tests {
         .changed()
         .unwrap();
         let mut pool = test_async!(engine.get_mut_pool(PoolIdentifier::Uuid(uuid))).unwrap();
-        assert!(match pool.destroy_filesystems(pool_name, &[]) {
+        assert!(match pool.destroy_filesystems(pool_name, &HashSet::new()) {
             Ok(uuids) => !uuids.is_changed(),
             _ => false,
         });
@@ -930,7 +930,7 @@ mod tests {
         .unwrap();
         let mut pool = test_async!(engine.get_mut_pool(PoolIdentifier::Uuid(uuid))).unwrap();
         assert_matches!(
-            pool.destroy_filesystems(pool_name, &[FilesystemUuid::new_v4()]),
+            pool.destroy_filesystems(pool_name, &[FilesystemUuid::new_v4()].into()),
             Ok(_)
         );
     }
@@ -955,10 +955,12 @@ mod tests {
             .changed()
             .unwrap();
         let fs_uuid = fs_results[0].1;
-        assert!(match pool.destroy_filesystems(pool_name, &[fs_uuid]) {
-            Ok(filesystems) => filesystems == SetDeleteAction::new(vec![fs_uuid], vec![]),
-            _ => false,
-        });
+        assert!(
+            match pool.destroy_filesystems(pool_name, &[fs_uuid].into()) {
+                Ok(filesystems) => filesystems == SetDeleteAction::new(vec![fs_uuid], vec![]),
+                _ => false,
+            }
+        );
     }
 
     #[test]

diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs
@@ -953,20 +953,18 @@ mod test {
             let mut pool = test_async!(engine.get_mut_pool(PoolIdentifier::Uuid(uuid1))).unwrap();
             pool.destroy_filesystems(
                 name2,
-                fs_uuid1
+                &fs_uuid1
                     .into_iter()
                     .map(|(_, u, _)| u)
-                    .collect::<Vec<_>>()
-                    .as_slice(),
+                    .collect::<HashSet<_>>(),
             )
             .unwrap();
             pool.destroy_filesystems(
                 name2,
-                fs_uuid2
+                &fs_uuid2
                     .into_iter()
                     .map(|(_, u, _)| u)
-                    .collect::<Vec<_>>()
-                    .as_slice(),
+                    .collect::<HashSet<_>>(),
             )
             .unwrap();
         }

diff --git a/src/engine/strat_engine/pool/dispatch.rs b/src/engine/strat_engine/pool/dispatch.rs
@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::path::Path;
+use std::{collections::HashSet, path::Path};
 
 use serde_json::Value;
 
@@ -122,7 +122,7 @@ impl Pool for AnyPool {
     fn destroy_filesystems(
         &mut self,
         pool_name: &str,
-        fs_uuids: &[FilesystemUuid],
+        fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
         match self {
             AnyPool::V1(p) => p.destroy_filesystems(pool_name, fs_uuids),

diff --git a/src/engine/strat_engine/pool/v1.rs b/src/engine/strat_engine/pool/v1.rs
@@ -2,7 +2,12 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::{cmp::max, collections::HashMap, path::Path, vec::Vec};
+use std::{
+    cmp::max,
+    collections::{HashMap, HashSet},
+    path::Path,
+    vec::Vec,
+};
 
 use chrono::{DateTime, Utc};
 use serde_json::{Map, Value};
@@ -1016,7 +1021,7 @@ impl Pool for StratPool {
     fn destroy_filesystems(
         &mut self,
         pool_name: &str,
-        fs_uuids: &[FilesystemUuid],
+        fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
         let mut removed = Vec::new();
         for &uuid in fs_uuids {
@@ -1696,7 +1701,8 @@ mod tests {
         assert!(pool
             .set_overprov_mode(&Name::new(pool_name.to_string()), false)
             .is_err());
-        pool.destroy_filesystems(pool_name, &[fs_uuid]).unwrap();
+        pool.destroy_filesystems(pool_name, &[fs_uuid].into())
+            .unwrap();
 
         pool.set_overprov_mode(&Name::new(pool_name.to_string()), false)
             .unwrap();

diff --git a/src/engine/strat_engine/pool/v2.rs b/src/engine/strat_engine/pool/v2.rs
@@ -2,7 +2,11 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::{collections::HashMap, path::Path, vec::Vec};
+use std::{
+    collections::{HashMap, HashSet},
+    path::Path,
+    vec::Vec,
+};
 
 use chrono::{DateTime, Utc};
 use serde_json::{Map, Value};
@@ -922,7 +926,7 @@ impl Pool for StratPool {
     fn destroy_filesystems(
         &mut self,
         pool_name: &str,
-        fs_uuids: &[FilesystemUuid],
+        fs_uuids: &HashSet<FilesystemUuid>,
     ) -> StratisResult<SetDeleteAction<FilesystemUuid, FilesystemUuid>> {
         let mut removed = Vec::new();
         for &uuid in fs_uuids {
@@ -1603,7 +1607,8 @@ mod tests {
         assert!(pool
             .set_overprov_mode(&Name::new(pool_name.to_string()), false)
             .is_err());
-        pool.destroy_filesystems(pool_name, &[fs_uuid]).unwrap();
+        pool.destroy_filesystems(pool_name, &[fs_uuid].into())
+            .unwrap();
 
         pool.set_overprov_mode(&Name::new(pool_name.to_string()), false)
             .unwrap();

diff --git a/src/jsonrpc/server/filesystem.rs b/src/jsonrpc/server/filesystem.rs
@@ -71,7 +71,11 @@ pub async fn filesystem_destroy<'a>(
     let (uuid, _) = pool
         .get_filesystem_by_name(&Name::new(fs_name.to_string()))
         .ok_or_else(|| StratisError::Msg(format!("No filesystem named {fs_name} found")))?;
-    block_in_place(|| Ok(pool.destroy_filesystems(pool_name, &[uuid])?.is_changed()))
+    block_in_place(|| {
+        Ok(pool
+            .destroy_filesystems(pool_name, &[uuid].into())?
+            .is_changed())
+    })
 }
 
 // stratis-min filesystem rename