review comments

* Converted FileSystemContextRule to try_from
* Added more error checking
* formatting and typo fixes

data/error_policies/fs_context.cas

@@ -5,7 +5,10 @@ resource foo {
     fs_context(this, "proc", zap);
 
     fs_context(this, "sysfs", genfscon, "/zap", [bar]);
+    fs_context(this, "sysfs", genfscon, "/zap", [file bar]);
+    fs_context(this, "fs1", xattr, "/zap", [file dir]);
+    fs_context(this, "fs2", task, "/zap");
 
-	// Policies must include at least one av rule
-	allow(domain, foo, file, [read]);
+    // Policies must include at least one av rule
+    allow(domain, foo, file, [read]);
 }

data/error_policies/fs_context_dup.cas

@@ -7,8 +7,8 @@ resource foo {
 
     fs_context(this, "test", genfscon, "/zap/baa", [file]);
 
-	// Policies must include at least one av rule
-	allow(domain, foo, file, [read]);
+    // Policies must include at least one av rule
+    allow(domain, foo, file, [read]);
 }
 
 resource bar {

data/policies/fs_context.cas

@@ -10,8 +10,8 @@ resource foo {
     // TODO re-add when secilc check is in place
     // fs_context(this, "sysfs", genfscon, "/zap", [dir]);
 
-	// Policies must include at least one av rule
-	allow(domain, foo, file, [read]);
+    // Policies must include at least one av rule
+    allow(domain, foo, file, [read]);
 }
 
 // TODO re-add when secilc check is in place

src/compile.rs

@@ -49,7 +49,7 @@ pub fn generate_sexp(
     // TODO: The rest of compilation
     let cil_types = type_list_to_sexp(type_decl_list, type_map);
     let headers = generate_cil_headers(classlist, *system_configurations);
-    let cil_rules = rules_list_to_sexp(policy_rules);
+    let cil_rules = rules_list_to_sexp(policy_rules)?;
     let cil_macros = func_map_to_sexp(func_map)?;
     let sid_statements =
         generate_sid_rules(generate_sids("kernel_sid", "security_sid", "unlabeled_sid"));
@@ -1509,11 +1509,12 @@ fn get_rules_vec_for_type(ti: &TypeInfo, s: sexp::Sexp, type_map: &TypeMap) -> V
     ret
 }
 
-fn rules_list_to_sexp<'a, T>(rules: T) -> Vec<sexp::Sexp>
+fn rules_list_to_sexp<'a, T>(rules: T) -> Result<Vec<sexp::Sexp>, ErrorItem>
 where
     T: IntoIterator<Item = ValidatedStatement<'a>>,
 {
-    rules.into_iter().map(|r| Sexp::from(&r)).collect()
+    let ret: Result<Vec<_>, _> = rules.into_iter().map(|r| Sexp::try_from(&r)).collect();
+    ret
 }
 
 fn generate_sids<'a>(

src/internal_rep.rs

@@ -18,7 +18,7 @@ use crate::ast::{
 };
 use crate::constants;
 use crate::context::Context as BlockContext;
-use crate::error::{CascadeErrors, CompileError, ErrorItem, InternalError};
+use crate::error::{CascadeErrors, CompileError, ErrorItem, InternalError, InvalidFileSystemError};
 use crate::obj_class::perm_list_to_sexp;
 
 const DEFAULT_USER: &str = "system_u";
@@ -1393,23 +1393,18 @@ pub struct FileSystemContextRule<'a> {
     pub context: Context<'a>,
 }
 
-// TODO convert to TryFrom/try_from see comment below
-impl From<&FileSystemContextRule<'_>> for sexp::Sexp {
-    fn from(f: &FileSystemContextRule) -> sexp::Sexp {
+impl TryFrom<&FileSystemContextRule<'_>> for sexp::Sexp {
+    type Error = ErrorItem;
+
+    fn try_from(f: &FileSystemContextRule) -> Result<sexp::Sexp, ErrorItem> {
         match f.fscontext_type {
-            FSContextType::XAttr | FSContextType::Task | FSContextType::Trans => list(&[
+            FSContextType::XAttr | FSContextType::Task | FSContextType::Trans => Ok(list(&[
                 atom_s("fsuse"),
                 Sexp::Atom(Atom::S(f.fscontext_type.to_string())),
                 atom_s(f.fs_name.trim_matches('"')),
                 Sexp::from(&f.context),
-            ]),
+            ])),
             FSContextType::GenFSCon => {
-                // Since path is an optional arg and I don't want to get
-                // into unwrap issue we are doing an 'if let' here.  The lack
-                // of path should be caught earlier, so if we don't have a path
-                // we will return an empty list.  The more correct way to fix this
-                // is convert this to a try_from, but this causes issues with some
-                // of our match statements and mixing returns.
                 if let Some(p) = &f.path {
                     if let Some(file_type) = &f.file_type {
                         // TODO add secilc check here. Right now our github pipeline
@@ -1418,25 +1413,31 @@ impl From<&FileSystemContextRule<'_>> for sexp::Sexp {
                         // REMEMBER TO UPDATE THE TESTS
                         // if secilc/libsepol version is new enough {
                         if false {
-                            return list(&[
+                            return Ok(list(&[
                                 atom_s("genfscon"),
                                 atom_s(f.fs_name.trim_matches('"')),
                                 atom_s(p.as_ref()),
                                 Sexp::Atom(Atom::S(file_type.to_string())),
                                 Sexp::from(&f.context),
-                            ]);
+                            ]));
                         }
                     }
                     // We are purposefully falling through without an else to
                     // reduce redundant lines of code
-                    list(&[
+                    Ok(list(&[
                         atom_s("genfscon"),
                         atom_s(f.fs_name.trim_matches('"')),
                         atom_s(p.as_ref()),
                         Sexp::from(&f.context),
-                    ])
+                    ]))
                 } else {
-                    list(&[])
+                    Err(ErrorItem::InvalidFileSystem(InvalidFileSystemError::new(
+                        &format!(
+                            "Genfscon missing path.\n No path given for genfscon rule:\
+                        \n\tFilesystem name: {}\n\tContext: {}",
+                            f.fs_name, f.context,
+                        ),
+                    )))
                 }
             }
         }
@@ -1536,30 +1537,49 @@ fn call_to_fsc_rules<'a>(
             return Err(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
                 "Not a valid file system type",
                 file,
-                fscontext_str.get_range(), //TODO error not showing correctly
+                fscontext_str.get_range(),
                 "File system type must be 'xattr', 'task', 'trans', or 'genfscon'",
             ))));
         }
     };
-    let regex_string = args_iter
+    let regex_string_arg = args_iter
         .next()
-        .ok_or_else(|| ErrorItem::Internal(InternalError::new()))?
-        .get_name_or_string(context)?
-        .to_string();
-    let file_types = args_iter
+        .ok_or_else(|| ErrorItem::Internal(InternalError::new()))?;
+    let regex_string = regex_string_arg.get_name_or_string(context)?.to_string();
+    let file_types_arg = args_iter
         .next()
-        .ok_or_else(|| ErrorItem::Internal(InternalError::new()))?
-        .get_list(context)?;
+        .ok_or_else(|| ErrorItem::Internal(InternalError::new()))?;
+    let file_types = file_types_arg.get_list(context)?;
 
     match fscontext_type {
         FSContextType::XAttr | FSContextType::Task | FSContextType::Trans => {
-            ret.push(FileSystemContextRule {
-                fscontext_type,
-                fs_name,
-                path: None,
-                file_type: None,
-                context: fs_context.clone(),
-            });
+            // The 'regex_string_arg.get_range().is_none()' is a hacky way to
+            // to check if arg was actually provided or not.  Since we set the
+            // default for regex_string to "/" this is the only way I could find
+            // to test if the actual arg was passed or not
+            if regex_string_arg.get_range().is_none() && file_types.is_empty() {
+                ret.push(FileSystemContextRule {
+                    fscontext_type,
+                    fs_name,
+                    path: None,
+                    file_type: None,
+                    context: fs_context.clone(),
+                });
+            } else if !file_types.is_empty() {
+                return Err(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(
+                    "File path can only be provided for 'genfscon'",
+                    file,
+                    regex_string_arg.get_range(),
+                    "",
+                ))));
+            }
         }
         FSContextType::GenFSCon => {
             if file_types.is_empty() {
@@ -1967,7 +1987,7 @@ impl TryFrom<&FunctionInfo<'_>> for sexp::Sexp {
                         ValidatedStatement::Call(c) => macro_cil.push(Sexp::from(&**c)),
                         ValidatedStatement::AvRule(a) => macro_cil.push(Sexp::from(a)),
                         ValidatedStatement::FcRule(f) => macro_cil.push(Sexp::from(f)),
-                        ValidatedStatement::FscRule(fs) => macro_cil.push(Sexp::from(fs)),
+                        ValidatedStatement::FscRule(fs) => macro_cil.push(Sexp::try_from(fs)?),
                         ValidatedStatement::DomtransRule(d) => macro_cil.push(Sexp::from(d)),
                     }
                 }
@@ -2094,7 +2114,7 @@ impl<'a> ValidatedStatement<'a> {
                             .collect())
                     } else {
                         Err(CascadeErrors::from(ErrorItem::Compile(CompileError::new(
-                            "file_context() calls are only allowed in resources",
+                            "fs_context() calls are only allowed in resources",
                             file,
                             c.name.get_range(),
                             "Not allowed here",
@@ -2157,14 +2177,15 @@ impl<'a> ValidatedStatement<'a> {
     }
 }
 
-impl From<&ValidatedStatement<'_>> for sexp::Sexp {
-    fn from(statement: &ValidatedStatement) -> sexp::Sexp {
+impl TryFrom<&ValidatedStatement<'_>> for sexp::Sexp {
+    type Error = ErrorItem;
+    fn try_from(statement: &ValidatedStatement) -> Result<sexp::Sexp, ErrorItem> {
         match statement {
-            ValidatedStatement::Call(c) => Sexp::from(&**c),
-            ValidatedStatement::AvRule(a) => Sexp::from(a),
-            ValidatedStatement::FcRule(f) => Sexp::from(f),
-            ValidatedStatement::FscRule(fs) => Sexp::from(fs),
-            ValidatedStatement::DomtransRule(d) => Sexp::from(d),
+            ValidatedStatement::Call(c) => Ok(Sexp::from(&**c)),
+            ValidatedStatement::AvRule(a) => Ok(Sexp::from(a)),
+            ValidatedStatement::FcRule(f) => Ok(Sexp::from(f)),
+            ValidatedStatement::FscRule(fs) => Sexp::try_from(fs),
+            ValidatedStatement::DomtransRule(d) => Ok(Sexp::from(d)),
         }
     }
 }

src/lib.rs

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