Merge pull request #3583 from mulkieran/enumerate-feature-args

Use strum to eliminate boilerplate from a bunch of enums
stratis-storage · Nov 14, 2024 · 1f0fe7b · 1f0fe7b
2 parents 677593e + d34b83d
commit 1f0fe7b
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 156 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -213,6 +213,14 @@ version = "0.2.1"
 optional = true
 path = "./stratisd_proc_macros"
 
+[dependencies.strum]
+version = "0.26.0"
+optional = true
+
+[dependencies.strum_macros]
+version = "0.26.0"
+optional = true
+
 [dependencies.tempfile]
 version = "3.4.0"
 optional = true
@@ -279,6 +287,8 @@ engine = [
     "dep:serde_json",
     "dep:sha2",
     "dep:stratisd_proc_macros",
+    "dep:strum",
+    "dep:strum_macros",
     "dep:tempfile",
     "dep:tokio",
     "dep:uuid"

diff --git a/src/bin/stratis-min/stratis-min.rs b/src/bin/stratis-min/stratis-min.rs
@@ -13,7 +13,7 @@ use stratisd::{
         CLEVIS_TANG_TRUST_URL,
     },
     jsonrpc::client::{filesystem, key, pool, report},
-    stratis::VERSION,
+    stratis::{StratisError, VERSION},
 };
 
 fn parse_args() -> Command {
@@ -237,7 +237,9 @@ fn main() -> Result<(), String> {
                 };
                 let unlock_method =
                     match args.get_one::<String>("unlock_method").map(|s| s.as_str()) {
-                        Some(um) => Some(UnlockMethod::try_from(um)?),
+                        Some(um) => Some(UnlockMethod::try_from(um).map_err(|_| {
+                            StratisError::Msg(format!("{um} is an invalid unlock method"))
+                        })?),
                         None => None,
                     };
                 let prompt = args.get_flag("prompt");

diff --git a/src/dbus_api/api/manager_3_0/methods.rs b/src/dbus_api/api/manager_3_0/methods.rs
@@ -179,7 +179,9 @@ pub fn unlock_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     };
     let unlock_method = {
         let unlock_method_str: &str = get_next_arg(&mut iter, 1)?;
-        match UnlockMethod::try_from(unlock_method_str) {
+        match UnlockMethod::try_from(unlock_method_str).map_err(|_| {
+            StratisError::Msg(format!("{unlock_method_str} is an invalid unlock method"))
+        }) {
             Ok(um) => um,
             Err(e) => {
                 let (rc, rs) = engine_to_dbus_err_tuple(&e);

diff --git a/src/dbus_api/api/manager_3_2/methods.rs b/src/dbus_api/api/manager_3_2/methods.rs
@@ -46,13 +46,17 @@ pub fn start_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     let unlock_method = {
         let unlock_method_tup: (bool, &str) = get_next_arg(&mut iter, 1)?;
         match tuple_to_option(unlock_method_tup) {
-            Some(unlock_method_str) => match UnlockMethod::try_from(unlock_method_str) {
-                Ok(um) => Some(um),
-                Err(e) => {
-                    let (rc, rs) = engine_to_dbus_err_tuple(&e);
-                    return Ok(vec![return_message.append3(default_return, rc, rs)]);
+            Some(unlock_method_str) => {
+                match UnlockMethod::try_from(unlock_method_str).map_err(|_| {
+                    StratisError::Msg(format!("{unlock_method_str} is an invalid unlock method"))
+                }) {
+                    Ok(um) => Some(um),
+                    Err(e) => {
+                        let (rc, rs) = engine_to_dbus_err_tuple(&e);
+                        return Ok(vec![return_message.append3(default_return, rc, rs)]);
+                    }
                 }
-            },
+            }
             None => None,
         }
     };

diff --git a/src/dbus_api/api/manager_3_4/methods.rs b/src/dbus_api/api/manager_3_4/methods.rs
@@ -52,13 +52,17 @@ pub fn start_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     let unlock_method = {
         let unlock_method_tup: (bool, &str) = get_next_arg(&mut iter, 2)?;
         match tuple_to_option(unlock_method_tup) {
-            Some(unlock_method_str) => match UnlockMethod::try_from(unlock_method_str) {
-                Ok(um) => Some(um),
-                Err(e) => {
-                    let (rc, rs) = engine_to_dbus_err_tuple(&e);
-                    return Ok(vec![return_message.append3(default_return, rc, rs)]);
+            Some(unlock_method_str) => {
+                match UnlockMethod::try_from(unlock_method_str).map_err(|_| {
+                    StratisError::Msg(format!("{unlock_method_str} is an invalid unlock method"))
+                }) {
+                    Ok(um) => Some(um),
+                    Err(e) => {
+                        let (rc, rs) = engine_to_dbus_err_tuple(&e);
+                        return Ok(vec![return_message.append3(default_return, rc, rs)]);
+                    }
                 }
-            },
+            }
             None => None,
         }
     };

diff --git a/src/dbus_api/api/manager_3_8/methods.rs b/src/dbus_api/api/manager_3_8/methods.rs
@@ -52,13 +52,17 @@ pub fn start_pool(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     let unlock_method = {
         let unlock_method_tup: (bool, &str) = get_next_arg(&mut iter, 2)?;
         match tuple_to_option(unlock_method_tup) {
-            Some(unlock_method_str) => match UnlockMethod::try_from(unlock_method_str) {
-                Ok(um) => Some(um),
-                Err(e) => {
-                    let (rc, rs) = engine_to_dbus_err_tuple(&e);
-                    return Ok(vec![return_message.append3(default_return, rc, rs)]);
+            Some(unlock_method_str) => {
+                match UnlockMethod::try_from(unlock_method_str).map_err(|_| {
+                    StratisError::Msg(format!("{unlock_method_str} is an invalid unlock method"))
+                }) {
+                    Ok(um) => Some(um),
+                    Err(e) => {
+                        let (rc, rs) = engine_to_dbus_err_tuple(&e);
+                        return Ok(vec![return_message.append3(default_return, rc, rs)]);
+                    }
                 }
-            },
+            }
             None => None,
         }
     };

diff --git a/src/dbus_api/api/report_3_0/methods.rs b/src/dbus_api/api/report_3_0/methods.rs
@@ -11,6 +11,7 @@ use crate::{
         util::{engine_to_dbus_err_tuple, get_next_arg},
     },
     engine::ReportType,
+    stratis::StratisError,
 };
 
 pub fn get_report(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
@@ -21,7 +22,9 @@ pub fn get_report(m: &MethodInfo<'_, MTSync<TData>, TData>) -> MethodResult {
     let return_message = message.method_return();
     let default_return = String::new();
 
-    let report_type = match ReportType::try_from(report_name) {
+    let report_type = match ReportType::try_from(report_name)
+        .map_err(|_| StratisError::Msg(format!("Report name {report_name} not understood")))
+    {
         Ok(rt) => rt,
         Err(e) => {
             let (rc, rs) = engine_to_dbus_err_tuple(&e);

diff --git a/src/engine/strat_engine/metadata/static_header.rs b/src/engine/strat_engine/metadata/static_header.rs
@@ -497,7 +497,7 @@ impl StaticHeader {
         let mut buf = [0u8; bytes!(static_header_size::SIGBLOCK_SECTORS)];
         buf[4..20].clone_from_slice(STRAT_MAGIC);
         LittleEndian::write_u64(&mut buf[20..28], *self.blkdev_size.sectors());
-        buf[28] = u8::from(self.sigblock_version);
+        buf[28] = self.sigblock_version as u8;
         buf[32..64].clone_from_slice(uuid_to_string!(self.identifiers.pool_uuid).as_bytes());
         buf[64..96].clone_from_slice(uuid_to_string!(self.identifiers.device_uuid).as_bytes());
         LittleEndian::write_u64(&mut buf[96..104], *self.mda_size.sectors());
@@ -532,7 +532,8 @@ impl StaticHeader {
         let blkdev_size = BlockdevSize::new(Sectors(LittleEndian::read_u64(&buf[20..28])));
 
         let version_buf = buf[28];
-        let version = StratSigblockVersion::try_from(version_buf)?;
+        let version = StratSigblockVersion::from_repr(version_buf)
+            .ok_or_else(|| StratisError::Msg(format!("Unknown sigblock version: {version_buf}")))?;
 
         let pool_uuid = PoolUuid::parse_str(from_utf8(&buf[32..64])?)?;
         let dev_uuid = DevUuid::parse_str(from_utf8(&buf[64..96])?)?;

diff --git a/src/engine/strat_engine/names.rs b/src/engine/strat_engine/names.rs
@@ -6,6 +6,8 @@
 
 use std::fmt::{self, Display};
 
+use strum_macros::{self};
+
 use devicemapper::{DmNameBuf, DmUuidBuf};
 
 pub use crate::engine::types::KeyDescription;
@@ -90,25 +92,18 @@ pub fn format_crypt_backstore_name(pool_uuid: &PoolUuid) -> DmNameBuf {
     DmNameBuf::new(value).expect("FORMAT_VERSION display length < 73")
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, strum_macros::Display)]
 pub enum FlexRole {
+    #[strum(serialize = "mdv")]
     MetadataVolume,
+    #[strum(serialize = "thindata")]
     ThinData,
+    #[strum(serialize = "thinmeta")]
     ThinMeta,
+    #[strum(serialize = "thinmetaspare")]
     ThinMetaSpare,
 }
 
-impl Display for FlexRole {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match *self {
-            FlexRole::MetadataVolume => write!(f, "mdv"),
-            FlexRole::ThinData => write!(f, "thindata"),
-            FlexRole::ThinMeta => write!(f, "thinmeta"),
-            FlexRole::ThinMetaSpare => write!(f, "thinmetaspare"),
-        }
-    }
-}
-
 #[derive(Clone, Copy)]
 pub enum ThinRole {
     Filesystem(FilesystemUuid),
@@ -122,21 +117,15 @@ impl Display for ThinRole {
     }
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, strum_macros::Display)]
+#[strum(serialize_all = "lowercase")]
 pub enum ThinPoolRole {
     Pool,
 }
 
-impl Display for ThinPoolRole {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match *self {
-            ThinPoolRole::Pool => write!(f, "pool"),
-        }
-    }
-}
-
 /// The various roles taken on by DM devices in the cache tier.
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, strum_macros::Display)]
+#[strum(serialize_all = "lowercase")]
 pub enum CacheRole {
     /// The DM cache device, contains the other three devices.
     Cache,
@@ -148,17 +137,6 @@ pub enum CacheRole {
     OriginSub,
 }
 
-impl Display for CacheRole {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match *self {
-            CacheRole::Cache => write!(f, "cache"),
-            CacheRole::CacheSub => write!(f, "cachesub"),
-            CacheRole::MetaSub => write!(f, "metasub"),
-            CacheRole::OriginSub => write!(f, "originsub"),
-        }
-    }
-}
-
 /// Format a name & uuid for the flex layer.
 ///
 /// Prerequisite: len(format!("{}", FORMAT_VERSION)