Require at least one blockdev for initialization actions

Currently we do not check that initialization actions like creating a pool or initializing the cache are provided at least one blockdev. Given that blockdevs are required for these actions, this PR adds a check at the top level of the engine to return a helpful error message if no blockdevs are given.
GuillaumeGomez · Feb 19, 2020 · b8305a5 · b8305a5
1 parent 43d084e
commit b8305a5
Show file tree

Hide file tree

Showing 17 changed files with 222 additions and 209 deletions.
diff --git a/src/engine/macros.rs b/src/engine/macros.rs
@@ -153,3 +153,10 @@ macro_rules! create_pool_generate_error_string {
         )
     };
 }
+
+#[cfg(test)]
+macro_rules! strs_to_paths {
+    ($slice:expr) => {
+        &$slice.iter().map(Path::new).collect::<Vec<_>>()
+    };
+}
diff --git a/src/engine/sim_engine/engine.rs b/src/engine/sim_engine/engine.rs
@@ -41,19 +41,27 @@ impl Engine for SimEngine {
         match self.pools.get_by_name(name) {
             Some((_, pool)) => create_pool_idempotent_or_err(pool, name, blockdev_paths),
             None => {
-                let device_set: HashSet<_, RandomState> = HashSet::from_iter(blockdev_paths);
-                let devices = device_set.into_iter().cloned().collect::<Vec<&Path>>();
-
-                let (pool_uuid, pool) = SimPool::new(&Rc::clone(&self.rdm), &devices, redundancy);
-
-                if self.rdm.borrow_mut().throw_die() {
-                    return Err(StratisError::Engine(ErrorEnum::Error, "X".into()));
+                if blockdev_paths.is_empty() {
+                    Err(StratisError::Engine(
+                        ErrorEnum::Invalid,
+                        "At least one blockdev is required to create a pool.".to_string(),
+                    ))
+                } else {
+                    let device_set: HashSet<_, RandomState> = HashSet::from_iter(blockdev_paths);
+                    let devices = device_set.into_iter().cloned().collect::<Vec<&Path>>();
+
+                    let (pool_uuid, pool) =
+                        SimPool::new(&Rc::clone(&self.rdm), &devices, redundancy);
+
+                    if self.rdm.borrow_mut().throw_die() {
+                        return Err(StratisError::Engine(ErrorEnum::Error, "X".into()));
+                    }
+
+                    self.pools
+                        .insert(Name::new(name.to_owned()), pool_uuid, pool);
+
+                    Ok(CreateAction::Created(pool_uuid))
                 }
-
-                self.pools
-                    .insert(Name::new(name.to_owned()), pool_uuid, pool);
-
-                Ok(CreateAction::Created(pool_uuid))
             }
         }
     }
@@ -181,7 +189,11 @@ mod tests {
     fn destroy_empty_pool() {
         let mut engine = SimEngine::default();
         let uuid = engine
-            .create_pool("name", &[], None)
+            .create_pool(
+                "name",
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -258,7 +270,11 @@ mod tests {
             .create_pool(name, &[Path::new("/s/d")], None)
             .unwrap();
         assert_matches!(
-            engine.create_pool(name, &[], None),
+            engine.create_pool(
+                name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            ),
             Err(StratisError::Engine(ErrorEnum::Invalid, _))
         );
     }
@@ -302,7 +318,11 @@ mod tests {
         let name = "name";
         let mut engine = SimEngine::default();
         let uuid = engine
-            .create_pool(name, &[], None)
+            .create_pool(
+                name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -317,7 +337,11 @@ mod tests {
     fn rename_happens() {
         let mut engine = SimEngine::default();
         let uuid = engine
-            .create_pool("old_name", &[], None)
+            .create_pool(
+                "old_name",
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -333,11 +357,21 @@ mod tests {
         let new_name = "new_name";
         let mut engine = SimEngine::default();
         let uuid = engine
-            .create_pool("old_name", &[], None)
+            .create_pool(
+                "old_name",
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
-        engine.create_pool(new_name, &[], None).unwrap();
+        engine
+            .create_pool(
+                new_name,
+                strs_to_paths!(["/dev/four", "/dev/five", "/dev/six"]),
+                None,
+            )
+            .unwrap();
         assert_matches!(
             engine.rename_pool(uuid, new_name),
             Err(StratisError::Engine(ErrorEnum::AlreadyExists, _))
@@ -349,7 +383,13 @@ mod tests {
     fn rename_no_op() {
         let new_name = "new_name";
         let mut engine = SimEngine::default();
-        engine.create_pool(new_name, &[], None).unwrap();
+        engine
+            .create_pool(
+                new_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
+            .unwrap();
         assert_matches!(
             engine.rename_pool(Uuid::new_v4(), new_name),
             Ok(RenameAction::NoSource)

diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs
@@ -112,6 +112,16 @@ impl Pool for SimPool {
         paths: &[&Path],
         tier: BlockDevTier,
     ) -> StratisResult<SetCreateAction<DevUuid>> {
+        if paths.is_empty() {
+            return if !self.has_cache() && tier == BlockDevTier::Cache {
+                Err(StratisError::Engine(
+                    ErrorEnum::Invalid,
+                    "At least one blockdev path is required to initialize a cache.".to_string(),
+                ))
+            } else {
+                Ok(SetCreateAction::new(vec![]))
+            };
+        }
         let devices: HashSet<_, RandomState> = HashSet::from_iter(paths);
 
         let device_pairs: Vec<_> = devices
@@ -332,7 +342,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -351,7 +365,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -376,7 +394,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -402,7 +424,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -421,7 +447,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -438,7 +468,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -455,7 +489,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -478,7 +516,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -493,7 +535,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -515,7 +561,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -535,7 +585,11 @@ mod tests {
         let mut engine = SimEngine::default();
         let pool_name = "pool_name";
         let uuid = engine
-            .create_pool(pool_name, &[], None)
+            .create_pool(
+                pool_name,
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();
@@ -555,7 +609,11 @@ mod tests {
     fn add_device_empty() {
         let mut engine = SimEngine::default();
         let uuid = engine
-            .create_pool("pool_name", &[], None)
+            .create_pool(
+                "pool_name",
+                strs_to_paths!(["/dev/one", "/dev/two", "/dev/three"]),
+                None,
+            )
             .unwrap()
             .changed()
             .unwrap();

diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs
@@ -205,12 +205,19 @@ impl Engine for StratEngine {
         match self.pools.get_by_name(name) {
             Some((_, pool)) => create_pool_idempotent_or_err(pool, name, blockdev_paths),
             None => {
-                let (uuid, pool) = StratPool::initialize(name, blockdev_paths, redundancy)?;
-
-                let name = Name::new(name.to_owned());
-                devlinks::pool_added(&name);
-                self.pools.insert(name, uuid, pool);
-                Ok(CreateAction::Created(uuid))
+                if blockdev_paths.is_empty() {
+                    Err(StratisError::Engine(
+                        ErrorEnum::Invalid,
+                        "At least one blockdev is required to create a pool.".to_string(),
+                    ))
+                } else {
+                    let (uuid, pool) = StratPool::initialize(name, blockdev_paths, redundancy)?;
+
+                    let name = Name::new(name.to_owned());
+                    devlinks::pool_added(&name);
+                    self.pools.insert(name, uuid, pool);
+                    Ok(CreateAction::Created(uuid))
+                }
             }
         }
     }

diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs
@@ -309,6 +309,16 @@ impl Pool for StratPool {
         paths: &[&Path],
         tier: BlockDevTier,
     ) -> StratisResult<SetCreateAction<DevUuid>> {
+        if paths.is_empty() {
+            return if !self.has_cache() && tier == BlockDevTier::Cache {
+                Err(StratisError::Engine(
+                    ErrorEnum::Invalid,
+                    "At least one blockdev path is required to initialize a cache.".to_string(),
+                ))
+            } else {
+                Ok(SetCreateAction::new(vec![]))
+            };
+        }
         let bdev_info = if tier == BlockDevTier::Cache {
             // If adding cache devices, must suspend the pool, since the cache
             // must be augmeneted with the new devices.

diff --git a/tests/client-dbus/src/stratisd_client_dbus/_stratisd_constants.py b/tests/client-dbus/src/stratisd_client_dbus/_stratisd_constants.py
@@ -31,3 +31,12 @@ class StratisdErrors(IntEnum):
     BUSY = 3
     INTERNAL_ERROR = 4
     NOT_FOUND = 5
+
+
+class BlockDevTiers(IntEnum):
+    """
+    Tier to which a blockdev device belongs.
+    """
+
+    Data = 0
+    Cache = 1
diff --git a/tests/client-dbus/tests/dbus/filesystem/test_fetch_properties.py b/tests/client-dbus/tests/dbus/filesystem/test_fetch_properties.py
@@ -21,7 +21,7 @@
 
 from .._misc import SimTestCase, device_name_list
 
-_DEVICE_STRATEGY = device_name_list()
+_DEVICE_STRATEGY = device_name_list(1)
 
 
 class FetchPropertiesTestCase(SimTestCase):

diff --git a/tests/client-dbus/tests/dbus/filesystem/test_properties.py b/tests/client-dbus/tests/dbus/filesystem/test_properties.py
@@ -24,7 +24,7 @@
 
 from .._misc import SimTestCase, device_name_list
 
-_DEVICE_STRATEGY = device_name_list()
+_DEVICE_STRATEGY = device_name_list(1)
 
 
 class SetNameTestCase(SimTestCase):

diff --git a/tests/client-dbus/tests/dbus/filesystem/test_rename.py b/tests/client-dbus/tests/dbus/filesystem/test_rename.py
@@ -29,7 +29,7 @@
 
 from .._misc import SimTestCase, device_name_list
 
-_DEVICE_STRATEGY = device_name_list()
+_DEVICE_STRATEGY = device_name_list(1)
 
 
 class SetNameTestCase(SimTestCase):