compat: Prioritize error over incompatibility

Reorder TryCompat::try_compat() checks to first call try_compat_inner()
and then also always call try_compat_children().  This ensures that
inner and children potential errors are always returned even if the
current object is incompatible.

For instance, this makes NetPort (see a following commit) able to check
port overflow even when the running kernel does not support ABI::V4.

Update the path_beneath_try_compat_children() test to reflect the error
ordering change:
- previously with try_compat_children() as first check and then
  try_compat_inner(),
- now with try_compat_inner() as first check and then
  try_compat_children().

This is now more consistent because the inner error is always returned
whatever there is a compatibility error or not.

Signed-off-by: Mickaël Salaün <[email protected]>

src/access.rs

@@ -52,34 +52,36 @@ impl<A> TryCompat<A> for BitFlags<A>
 where
     A: Access,
 {
-    fn try_compat_inner(self, abi: ABI) -> Result<CompatResult<Self, A>, CompatError<A>> {
+    fn try_compat_inner(&mut self, abi: ABI) -> Result<CompatResult<A>, CompatError<A>> {
         if self.is_empty() {
             // Empty access-rights would result to a runtime error.
             Err(AccessError::Empty.into())
-        } else if !Self::all().contains(self) {
+        } else if !Self::all().contains(*self) {
             // Unknown access-rights (at build time) would result to a runtime error.
             // This can only be reached by using the unsafe BitFlags::from_bits_unchecked().
             Err(AccessError::Unknown {
-                access: self,
-                unknown: self & full_negation(Self::all()),
+                access: *self,
+                unknown: *self & full_negation(Self::all()),
             }
             .into())
         } else {
-            let compat = self & A::from_all(abi);
-            if compat.is_empty() {
+            let compat = *self & A::from_all(abi);
+            let ret = if compat.is_empty() {
                 Ok(CompatResult::No(
-                    AccessError::Incompatible { access: self }.into(),
+                    AccessError::Incompatible { access: *self }.into(),
                 ))
-            } else if compat != self {
+            } else if compat != *self {
                 let error = AccessError::PartiallyCompatible {
-                    access: self,
-                    incompatible: self & full_negation(compat),
+                    access: *self,
+                    incompatible: *self & full_negation(compat),
                 }
                 .into();
-                Ok(CompatResult::Partial(compat, error))
+                Ok(CompatResult::Partial(error))
             } else {
-                Ok(CompatResult::Full(self))
-            }
+                Ok(CompatResult::Full)
+            };
+            *self = compat;
+            ret
         }
     }
 }

src/compat.rs

@@ -558,20 +558,15 @@ fn tailored_compat_level() {
     }
 }
 
-// CompatResult is useful because we don't want to duplicate objects (potentially wrapping a file
-// descriptor), and we may not have compatibility errors for some objects.  TryCompat::try_compat()
-// is responsible to either take T or CompatError<A> according to the compatibility level.
-//
 // CompatResult is not public outside this crate.
-pub enum CompatResult<T, A>
+pub enum CompatResult<A>
 where
-    T: TryCompat<A>,
     A: Access,
 {
     // Fully matches the request.
-    Full(T),
+    Full,
     // Partially matches the request.
-    Partial(T, CompatError<A>),
+    Partial(CompatError<A>),
     // Doesn't matches the request.
     No(CompatError<A>),
 }
@@ -582,14 +577,19 @@ where
     Self: Sized + TailoredCompatLevel,
     A: Access,
 {
-    fn try_compat_inner(self, abi: ABI) -> Result<CompatResult<Self, A>, CompatError<A>>;
+    fn try_compat_inner(&mut self, abi: ABI) -> Result<CompatResult<A>, CompatError<A>>;
 
     // Default implementation for objects without children.
     //
     // If returning something other than Ok(Some(self)), the implementation must use its own
     // compatibility level, if any, with self.tailored_compat_level(default_compat_level), and pass
     // it with the abi and compat_state to each child.try_compat().  See PathBeneath implementation
     // and the self.allowed_access.try_compat() call.
+    //
+    // # Warning
+    //
+    // Errors must be prioritized over incompatibility (i.e. return Err(e) over Ok(None)) for all
+    // children.
     fn try_compat_children<L>(
         self,
         _abi: ABI,
@@ -614,48 +614,51 @@ where
         L: Into<CompatLevel>,
     {
         let compat_level = self.tailored_compat_level(parent_level);
-        let new_self = match self.try_compat_children(abi, compat_level, compat_state)? {
-            Some(n) => n,
-            None => return Ok(None),
-        };
-        match new_self.try_compat_inner(abi) {
-            Ok(CompatResult::Full(new_self)) => {
+        let some_inner = match self.try_compat_inner(abi) {
+            Ok(CompatResult::Full) => {
                 compat_state.update(CompatState::Full);
-                Ok(Some(new_self))
+                true
             }
-            Ok(CompatResult::Partial(new_self, error)) => match compat_level {
+            Ok(CompatResult::Partial(error)) => match compat_level {
                 CompatLevel::BestEffort => {
                     compat_state.update(CompatState::Partial);
-                    Ok(Some(new_self))
+                    true
                 }
                 CompatLevel::SoftRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::HardRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Err(error)
+                    return Err(error);
                 }
             },
             Ok(CompatResult::No(error)) => match compat_level {
                 CompatLevel::BestEffort => {
                     compat_state.update(CompatState::No);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::SoftRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::HardRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Err(error)
+                    return Err(error);
                 }
             },
-            Err(e) => {
+            Err(error) => {
                 // Safeguard to help for test consistency.
                 compat_state.update(CompatState::Dummy);
-                Err(e)
+                return Err(error);
             }
+        };
+
+        // At this point, any inner error have been returned, so we can proceed with
+        // try_compat_children()?.
+        match self.try_compat_children(abi, compat_level, compat_state)? {
+            Some(n) if some_inner => Ok(Some(n)),
+            _ => Ok(None),
         }
     }
 }

src/fs.rs

@@ -269,11 +269,9 @@ where
     }
 
     fn try_compat_inner(
-        mut self,
+        &mut self,
         _abi: ABI,
-    ) -> Result<CompatResult<Self, AccessFs>, CompatError<AccessFs>> {
-        // self.allowed_access was updated with try_compat_children(), called by try_compat().
-
+    ) -> Result<CompatResult<AccessFs>, CompatError<AccessFs>> {
         // Gets subset of valid accesses according the FD type.
         let valid_access =
             if is_file(&self.parent_fd).map_err(|e| PathBeneathError::StatCall { source: e })? {
@@ -290,9 +288,9 @@ where
             .into();
             self.allowed_access = valid_access;
             // Linux would return EINVAL.
-            Ok(CompatResult::Partial(self, error))
+            Ok(CompatResult::Partial(error))
         } else {
-            Ok(CompatResult::Full(self))
+            Ok(CompatResult::Full)
         }
     }
 }
@@ -314,8 +312,8 @@ fn path_beneath_try_compat_children() {
             .add_rule(PathBeneath::new(PathFd::new("/dev/null").unwrap(), access_file))
             .unwrap_err(),
         RulesetError::AddRules(AddRulesError::Fs(AddRuleError::Compat(
-            CompatError::Access(AccessError::PartiallyCompatible { access, incompatible }))
-        )) if access == access_file && incompatible == AccessFs::Refer
+            CompatError::PathBeneath(PathBeneathError::DirectoryAccess { access, incompatible })
+        ))) if access == access_file && incompatible == AccessFs::Refer
     ));
 
     // Test error ordering with ABI::V2