Turn str_check function into a macro and put it in id_macros

No longer pass as an argument to str_id macro. I think str_check was defined
originally as a function because we were novice macro users. That function
was always passed for construction of the type, it was used exclusively for
that purpose, and it seems unlikely that it will need to be varied.

Have str_check just evaluate to an Option<String> rather than a Result.
Promote the Option to the error in str_id. Removes duplication and
isolates the use of the DmResult type to two places.

Signed-off-by: mulhern <amulhern@redhat.com>

Author: mulkieran

src/id_macros.rs

@@ -5,12 +5,35 @@
 // A module to contain functionality used for generating DM ids which
 // are restricted in length and format by devicemapper.
 
+// Evaluates to an error string if the value does not match the requirements.
+macro_rules! str_check {
+    ($value:expr, $max_allowed_chars:expr) => {{
+        let value = $value;
+        let max_allowed_chars = $max_allowed_chars;
+        if !value.is_ascii() {
+            Some(format!("value {} has some non-ascii characters", value))
+        } else {
+            let num_chars = value.len();
+            if num_chars == 0 {
+                Some("value has zero characters".into())
+            } else if num_chars > max_allowed_chars {
+                Some(format!(
+                    "value {} has {} chars which is greater than maximum allowed {}",
+                    value, num_chars, max_allowed_chars
+                ))
+            } else {
+                None
+            }
+        }
+    }};
+}
+
 /// Define borrowed and owned versions of string types that guarantee
 /// conformance to DM restrictions, such as maximum length.
 // This implementation follows the example of Path/PathBuf as closely as
 // possible.
 macro_rules! str_id {
-    ($B:ident, $O:ident, $MAX:ident, $check:ident) => {
+    ($B:ident, $O:ident, $MAX:ident) => {
         /// The borrowed version of the DM identifier.
         #[derive(Debug, PartialEq, Eq, Hash)]
         pub struct $B {
@@ -27,7 +50,11 @@ macro_rules! str_id {
             /// Create a new borrowed identifier from a `&str`.
             #[allow(clippy::new_ret_no_self)]
             pub fn new(value: &str) -> DmResult<&$B> {
-                $check(value, $MAX - 1)?;
+                if let Some(err_msg) = str_check!(value, $MAX - 1) {
+                    return Err(DmError::Core(
+                        ErrorKind::InvalidArgument(err_msg.into()).into(),
+                    ));
+                }
                 Ok(unsafe { &*(value as *const str as *const $B) })
             }
 
@@ -56,7 +83,11 @@ macro_rules! str_id {
             /// Construct a new owned identifier.
             #[allow(clippy::new_ret_no_self)]
             pub fn new(value: String) -> DmResult<$O> {
-                $check(&value, $MAX - 1)?;
+                if let Some(err_msg) = str_check!(&value, $MAX - 1) {
+                    return Err(DmError::Core(
+                        ErrorKind::InvalidArgument(err_msg.into()).into(),
+                    ));
+                }
                 Ok($O { inner: value })
             }
         }

src/types.rs

@@ -62,36 +62,14 @@ impl Sectors {
     }
 }
 
-/// Returns an error if value is unsuitable.
-fn str_check(value: &str, max_allowed_chars: usize) -> DmResult<()> {
-    if !value.is_ascii() {
-        let err_msg = format!("value {} has some non-ascii characters", value);
-        return Err(DmError::Core(ErrorKind::InvalidArgument(err_msg).into()));
-    }
-    let num_chars = value.len();
-    if num_chars == 0 {
-        return Err(DmError::Core(
-            ErrorKind::InvalidArgument("value has zero characters".into()).into(),
-        ));
-    }
-    if num_chars > max_allowed_chars {
-        let err_msg = format!(
-            "value {} has {} chars which is greater than maximum allowed {}",
-            value, num_chars, max_allowed_chars
-        );
-        return Err(DmError::Core(ErrorKind::InvalidArgument(err_msg).into()));
-    }
-    Ok(())
-}
-
 /// A devicemapper name. Really just a string, but also the argument type of
 /// DevId::Name. Used in function arguments to indicate that the function
 /// takes only a name, not a devicemapper uuid.
-str_id!(DmName, DmNameBuf, DM_NAME_LEN, str_check);
+str_id!(DmName, DmNameBuf, DM_NAME_LEN);
 
 /// A devicemapper uuid. A devicemapper uuid has a devicemapper-specific
 /// format.
-str_id!(DmUuid, DmUuidBuf, DM_UUID_LEN, str_check);
+str_id!(DmUuid, DmUuidBuf, DM_UUID_LEN);
 
 /// Used as a parameter for functions that take either a Device name
 /// or a Device UUID.
@@ -115,7 +93,7 @@ impl<'a> fmt::Display for DevId<'a> {
 /// Number of bytes in Struct_dm_target_spec::target_type field.
 const DM_TARGET_TYPE_LEN: usize = 16;
 
-str_id!(TargetType, TargetTypeBuf, DM_TARGET_TYPE_LEN, str_check);
+str_id!(TargetType, TargetTypeBuf, DM_TARGET_TYPE_LEN);
 
 #[cfg(test)]
 mod tests {