From d4c4e7b041c133cf0b9ea5e8d52fe993185f9f8a Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 15 Aug 2024 14:06:55 -0400 Subject: [PATCH] Enforce set invariant in destroy_filesystems Use HashSet type to enforce the set property. This allows more precise algorithms than the Vec type. Signed-off-by: mulhern --- src/dbus_api/pool/pool_3_0/methods.rs | 4 ++-- src/dbus_api/pool/pool_3_7/methods.rs | 4 ++-- src/engine/engine.rs | 2 +- src/engine/sim_engine/pool.rs | 16 +++++++++------- src/engine/strat_engine/engine.rs | 10 ++++------ src/engine/strat_engine/pool/dispatch.rs | 4 ++-- src/engine/strat_engine/pool/v1.rs | 12 +++++++++--- src/engine/strat_engine/pool/v2.rs | 11 ++++++++--- src/jsonrpc/server/filesystem.rs | 6 +++++- 9 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/dbus_api/pool/pool_3_0/methods.rs b/src/dbus_api/pool/pool_3_0/methods.rs index 38d676ad75..69fb72eb36 100644 --- 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>) -> MethodRe let result = handle_action!( pool.destroy_filesystems( &pool_name, - &filesystem_map.keys().cloned().collect::>(), + &filesystem_map.keys().cloned().collect::>(), ), 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 index 6508e974be..8f61dec9c3 100644 --- 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>) -> MethodRe let result = handle_action!( pool.destroy_filesystems( &pool_name, - &filesystem_map.keys().cloned().collect::>(), + &filesystem_map.keys().cloned().collect::>(), ), dbus_context, pool_path.get_name() diff --git a/src/engine/engine.rs b/src/engine/engine.rs index 5a72a6d9bb..4f96493cf5 100644 --- 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, ) -> StratisResult>; /// Rename filesystem diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs index 71e9dc6953..8676a82819 100644 --- 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, ) -> StratisResult> { 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 index ff801c4c89..c539805a5d 100644 --- 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::>() - .as_slice(), + .collect::>(), ) .unwrap(); pool.destroy_filesystems( name2, - fs_uuid2 + &fs_uuid2 .into_iter() .map(|(_, u, _)| u) - .collect::>() - .as_slice(), + .collect::>(), ) .unwrap(); } diff --git a/src/engine/strat_engine/pool/dispatch.rs b/src/engine/strat_engine/pool/dispatch.rs index c1baf3d45f..a99db97e4b 100644 --- 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, ) -> StratisResult> { 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 index 6c2506c5c5..d90e268d2c 100644 --- 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, ) -> StratisResult> { 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 index f8af4f4696..2a45991d76 100644 --- 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, ) -> StratisResult> { 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 index 691ebabcf4..e40fcaebd8 100644 --- 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