additional review comments

Author: matt-sheets

doc/TE.md

@@ -310,16 +310,16 @@ file_type [obj_class])`
   Currently this is only valid for the proc filesystem, all other types must be
   "/".  If not given, the field will default to "/".
 * `file_type` is an optional keyword representing a file type to apply the label
-  to. Valid values are the same as in file_context function.  If not given,
+  to. Valid values are the same as in the file_context function.  If not given,
   [any] is assumed.
   * Note: You must use SELinux userspace tools version 3.4 or newer to use this
     field.
 
 `xattr`, `task`, and `trans` all represent filesystems that support SELinux
 security contexts.  The filesystem itself has a labeled applied to it as a
-whole, which is the `fs_label` provided in this function.  All files on the
-filesystem also store their own SELinux security context in their own extended
-attributes. 
+whole, which is the `fs_label` provided in this function.  Individual files
+may also have specific security contexts stored in their extended attributes
+if supported by the filesystem.
 
 `genfscon` represents filesystems that do not support SELinux security contexts.
 Generally a filesystem has a single default security context, `fs_label`

src/compile.rs

@@ -339,8 +339,9 @@ pub fn build_func_map<'a>(
     Ok(decl_map)
 }
 
-#[allow(clippy::collapsible_if)]
-pub fn validate_rules(statements: &BTreeSet<ValidatedStatement>) -> Result<(), CascadeErrors> {
+pub fn validate_fs_context_duplicates(
+    statements: &BTreeSet<ValidatedStatement>,
+) -> Result<(), CascadeErrors> {
     let mut errors = CascadeErrors::new();
     let mut fsc_rules: BTreeMap<&String, BTreeSet<&FileSystemContextRule>> = BTreeMap::new();
 
@@ -368,7 +369,7 @@ pub fn validate_rules(statements: &BTreeSet<ValidatedStatement>) -> Result<(), C
                     continue 'key_loop;
                 }
                 FSContextType::GenFSCon => {
-                    // genfscon gets more complicated.  We can have simlar rules as long as the paths different.
+                    // genfscon gets more complicated.  We can have similar rules as long as the paths are different.
                     // If we find a genfscon with the same path, they must have the same context and object type.
                     if let Some(path) = &rule.path {
                         for inner_rule in &v {
@@ -383,25 +384,19 @@ pub fn validate_rules(statements: &BTreeSet<ValidatedStatement>) -> Result<(), C
                                         inner_rule.context,
                                     ))));
                                     continue 'key_loop;
-                                // else if let is not suppored currently (https://github.com/rust-lang/rust/issues/53667).
-                                // Thus we check if we are not none and then unwrap
                                 } else if path == inner_path
+                                    && rule.file_type != inner_rule.file_type
                                     && rule.file_type.is_some()
-                                    && inner_rule.file_type.is_some()
                                 {
-                                    if rule.file_type.as_ref().unwrap()
-                                        != inner_rule.file_type.as_ref().unwrap()
-                                    {
-                                        errors.append(CascadeErrors::from(InvalidFileSystemError::new(&format!(
-                                            "Duplicate genfscon.\n Found duplicate genfscon rules with differing object types: {}\
-                                            \n\tPath: {}\n\tObject Type 1: {}\n\tObject Type 2: {}",
-                                            rule.fs_name,
-                                            path,
-                                            rule.file_type.as_ref().unwrap(),
-                                            inner_rule.file_type.as_ref().unwrap(),
-                                        ))));
-                                        continue 'key_loop;
-                                    }
+                                    errors.append(CascadeErrors::from(InvalidFileSystemError::new(&format!(
+                                        "Duplicate genfscon.\n Found duplicate genfscon rules with differing object types: {}\
+                                        \n\tPath: {}\n\tObject Type 1: {}\n\tObject Type 2: {}",
+                                        rule.fs_name,
+                                        path,
+                                        rule.file_type.as_ref().unwrap(),
+                                        inner_rule.file_type.as_ref().unwrap(),
+                                    ))));
+                                    continue 'key_loop;
                                 }
                             }
                         }
@@ -410,7 +405,15 @@ pub fn validate_rules(statements: &BTreeSet<ValidatedStatement>) -> Result<(), C
             }
         }
     }
+    errors.into_result(())
+}
+
+pub fn validate_rules(statements: &BTreeSet<ValidatedStatement>) -> Result<(), CascadeErrors> {
+    let mut errors = CascadeErrors::new();
 
+    if let Err(call_errors) = validate_fs_context_duplicates(statements) {
+        errors.append(call_errors);
+    }
     errors.into_result(())
 }
 

src/error.rs

@@ -301,7 +301,7 @@ impl CascadeErrors {
         self.errors.push(error.into());
     }
 
-    fn is_empty(&self) -> bool {
+    pub fn is_empty(&self) -> bool {
         self.errors.is_empty()
     }
 

src/internal_rep.rs

@@ -1754,21 +1754,27 @@ fn call_to_fsc_rules<'a>(
                     file_type: None,
                     context: fs_context.clone(),
                 });
-            } else if !file_types.is_empty() {
-                return Err(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
+            }
+            let mut errors = CascadeErrors::new();
+            if !file_types.is_empty() {
+                errors.append(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
                     "File types can only be provided for 'genfscon'",
                     file,
                     file_types_arg.get_range(),
                     "",
                 ))));
-            } else {
-                return Err(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
+            }
+            if regex_string_arg.get_range().is_some() {
+                errors.append(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
                     "File path can only be provided for 'genfscon'",
                     file,
                     regex_string_arg.get_range(),
                     "",
                 ))));
             }
+            if !errors.is_empty() {
+                return Err(errors);
+            }
         }
         FSContextType::GenFSCon => {
             if file_types.is_empty() {

src/lib.rs

@@ -1189,7 +1189,7 @@ mod tests {
 
     #[test]
     fn invalid_fs_context() {
-        error_policy_test!("fs_context.cas", 8, ErrorItem::Compile(_));
+        error_policy_test!("fs_context.cas", 9, ErrorItem::Compile(_));
     }
 
     #[test]