[pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[Gabriel Goller]

Added `matches_path()` function, which only matches against the path and returns
an error if a file_mode pattern is found/needed in the matching list. This is
useful when we want to check if a file is excluded before running `stat()` on
the file to get the file_mode (which could fail).

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/match_list.rs | 159 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 158 insertions(+), 1 deletion(-)

diff --git a/src/match_list.rs b/src/match_list.rs
index c5b14e0..acad328 100644
--- a/src/match_list.rs
+++ b/src/match_list.rs
@@ -1,6 +1,6 @@
 //! Helpers for include/exclude lists.
-
 use bitflags::bitflags;
+use std::fmt;
 
 use crate::PatternFlag;
 
@@ -39,6 +39,17 @@ impl Default for MatchFlag {
     }
 }
 
+#[derive(Debug, PartialEq)]
+pub struct FileModeRequiredForMatching;
+
+impl fmt::Display for FileModeRequiredForMatching {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "File mode is required for matching")
+    }
+}
+
+impl std::error::Error for FileModeRequiredForMatching {}
+
 /// A pattern entry. (Glob patterns or literal patterns.)
 // Note:
 // For regex we'd likely use the POSIX extended REs via `regexec(3)`, since we're targetting
@@ -304,12 +315,32 @@ impl MatchEntry {
 
         self.matches_path_exact(path)
     }
+
+    /// Check whether the path contains a matching suffix. Returns an error if a file mode is required.
+    pub fn matches_path<T: AsRef<[u8]>>(
+        &self,
+        path: T,
+    ) -> Result<bool, FileModeRequiredForMatching> {
+        self.matches_path_do(path.as_ref())
+    }
+
+    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequiredForMatching> {
+        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
+            return Err(FileModeRequiredForMatching);
+        }
+
+        Ok(self.matches_path_suffix_do(path))
+    }
 }
 
 #[doc(hidden)]
 pub trait MatchListEntry {
     fn entry_matches(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
     fn entry_matches_exact(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
+    fn entry_matches_path(
+        &self,
+        path: &[u8],
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
 }
 
 impl MatchListEntry for &'_ MatchEntry {
@@ -328,6 +359,21 @@ impl MatchListEntry for &'_ MatchEntry {
             None
         }
     }
+
+    fn entry_matches_path(
+        &self,
+        path: &[u8],
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
+        if let Ok(b) = self.matches_path(path) {
+            if b {
+                Ok(Some(self.match_type()))
+            } else {
+                Ok(None)
+            }
+        } else {
+            Err(FileModeRequiredForMatching)
+        }
+    }
 }
 
 impl MatchListEntry for &'_ &'_ MatchEntry {
@@ -346,6 +392,21 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
             None
         }
     }
+
+    fn entry_matches_path(
+        &self,
+        path: &[u8],
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
+        if let Ok(b) = self.matches_path(path) {
+            if b {
+                Ok(Some(self.match_type()))
+            } else {
+                Ok(None)
+            }
+        } else {
+            Err(FileModeRequiredForMatching)
+        }
+    }
 }
 
 /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
@@ -374,6 +435,20 @@ pub trait MatchList {
     }
 
     fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
+
+    /// Check whether this list contains anything exactly matching the path, returns error if
+    /// `file_mode` is required for exact matching.
+    fn matches_path<T: AsRef<[u8]>>(
+        &self,
+        path: T,
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
+        self.matches_path_do(path.as_ref())
+    }
+
+    fn matches_path_do(
+        &self,
+        path: &[u8],
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
 }
 
 impl<'a, T> MatchList for T
@@ -408,6 +483,24 @@ where
 
         None
     }
+
+    fn matches_path_do(
+        &self,
+        path: &[u8],
+    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
+        // This is an &self method on a `T where T: 'a`.
+        let this: &'a Self = unsafe { std::mem::transmute(self) };
+
+        for m in this.into_iter().rev() {
+            if let Ok(mt) = m.entry_matches_path(path) {
+                if mt.is_some() {
+                    return Ok(mt);
+                }
+            }
+        }
+
+        Err(FileModeRequiredForMatching)
+    }
 }
 
 #[test]
@@ -530,3 +623,67 @@ fn test_path_relativity() {
     assert_eq!(matchlist.matches("foo/slash", None), None);
     assert_eq!(matchlist.matches("foo/slash-a", None), None);
 }
+
+#[test]
+fn test_matches_path() {
+    let vec = vec![
+        MatchEntry::include(crate::Pattern::path("as*").unwrap()),
+        MatchEntry::include(crate::Pattern::path("a*").unwrap()),
+    ];
+    assert_eq!(vec.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let list: &[MatchEntry] = &vec[..];
+    assert_eq!(list.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let list: Vec<&MatchEntry> = vec.iter().collect();
+    assert_eq!(list.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let list: &[&MatchEntry] = &list[..];
+    assert_eq!(list.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let vec = vec![
+        MatchEntry::include(crate::Pattern::path("a*").unwrap()),
+        MatchEntry::include(crate::Pattern::path("a*").unwrap())
+            .flags(MatchFlag::MATCH_REGULAR_FILES),
+    ];
+    assert_eq!(vec.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let vec = vec![
+        MatchEntry::include(crate::Pattern::path("a*").unwrap()),
+        MatchEntry::include(crate::Pattern::path("asdf").unwrap()),
+    ];
+    assert_eq!(vec.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let list: &[MatchEntry] = &vec[..];
+    assert_eq!(list.matches_path("azdf"), Ok(Some(MatchType::Include)));
+
+    let vec = vec![
+        MatchEntry::include(crate::Pattern::path("a*").unwrap()).flags(MatchFlag::ANY_FILE_TYPE),
+        MatchEntry::include(crate::Pattern::path("asdf").unwrap()).flags(MatchFlag::ANY_FILE_TYPE),
+    ];
+    assert_eq!(vec.matches_path("asdf"), Ok(Some(MatchType::Include)));
+    assert_eq!(vec.matches_path("adbb"), Ok(Some(MatchType::Include)));
+}
+
+#[test]
+fn test_matches_path_error() {
+    let vec = vec![
+        MatchEntry::include(crate::Pattern::path("a*").unwrap())
+            .flags(MatchFlag::MATCH_REGULAR_FILES),
+        MatchEntry::include(crate::Pattern::path("asdf").unwrap()),
+    ];
+    assert_eq!(vec.matches_path("asdf"), Ok(Some(MatchType::Include)));
+
+    let list: &[MatchEntry] = &vec[..];
+    assert_eq!(list.matches_path("azdf"), Err(FileModeRequiredForMatching));
+
+    let list: Vec<&MatchEntry> = vec.iter().collect();
+    assert_eq!(list.matches_path("abdf"), Err(FileModeRequiredForMatching));
+
+    let list: &[&MatchEntry] = &list[..];
+    assert_eq!(list.matches_path("acdf"), Err(FileModeRequiredForMatching));
+
+    let vec = vec![MatchEntry::include(crate::Pattern::path("a*").unwrap())
+        .flags(MatchFlag::MATCH_DIRECTORIES)];
+    assert_eq!(vec.matches_path("asdf"), Err(FileModeRequiredForMatching));
+}
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 1/1] fix #4380: check if file is excluded before running `stat()`

[Gabriel Goller]

Before running `stat()` we now make a `matches_path()` call, which checks
if the file/directory is excluded (`matches_path()` tries to match using
only the path, without the file mode). If the file is not excluded,
we `stat()` and then check again if the path matches using the file_mode and
the `matches()` function.
Added `pathpatterns` crate to local overrides in cargo.toml.

changes v2:
 - checking for `read` and `execute` permissions before entering directory,
   doesn't work because there are a lot of side-effects (executed by
   different user, AppArmor, SELinux, ...).
changes v1:
 - checking for excluded files with `matches()` before executing `stat()`,
   this doesn't work because we get the file_mode from `stat()` and don't 
   want to ignore it when matching.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 Cargo.toml                    |  1 +
 pbs-client/src/pxar/create.rs | 45 ++++++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 2a05c471..c54bc28b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -262,6 +262,7 @@ proxmox-rrd.workspace = true
 
 #proxmox-apt = { path = "../proxmox-apt" }
 #proxmox-openid = { path = "../proxmox-openid-rs" }
+#pathpatterns = {path = "../pathpatterns" }
 
 #pxar = { path = "../pxar" }
 
diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 2577cf98..a6d608e7 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -232,7 +232,7 @@ impl Archiver {
             let entry_counter = self.entry_counter;
 
             let old_patterns_count = self.patterns.len();
-            self.read_pxar_excludes(dir.as_raw_fd())?;
+            self.read_pxar_excludes(&mut dir)?;
 
             let mut file_list = self.generate_directory_file_list(&mut dir, is_root)?;
 
@@ -315,8 +315,23 @@ impl Archiver {
         }
     }
 
-    fn read_pxar_excludes(&mut self, parent: RawFd) -> Result<(), Error> {
-        let fd = match self.open_file(parent, c_str!(".pxarexclude"), OFlag::O_RDONLY, false)? {
+    fn read_pxar_excludes(&mut self, parent: &mut Dir) -> Result<(), Error> {
+        let mut exclude_file_found = false;
+        for file in parent.iter() {
+            if file?.file_name().to_str()? == ".pxarexclude" {
+                exclude_file_found = true;
+                break;
+            }
+        }
+        if !exclude_file_found {
+            return Ok(());
+        }
+        let fd = match self.open_file(
+            parent.as_raw_fd(),
+            c_str!(".pxarexclude"),
+            OFlag::O_RDONLY,
+            false,
+        )? {
             Some(fd) => fd,
             None => return Ok(()),
         };
@@ -420,7 +435,7 @@ impl Archiver {
         for file in dir.iter() {
             let file = file?;
 
-            let file_name = file.file_name().to_owned();
+            let file_name = file.file_name();
             let file_name_bytes = file_name.to_bytes();
             if file_name_bytes == b"." || file_name_bytes == b".." {
                 continue;
@@ -434,9 +449,17 @@ impl Archiver {
             assert_single_path_component(os_file_name)?;
             let full_path = self.path.join(os_file_name);
 
+            let match_path = PathBuf::from("/").join(full_path.clone());
+            let pre_stat_match = self
+                .patterns
+                .matches_path(match_path.as_os_str().as_bytes());
+            if pre_stat_match == Ok(Some(MatchType::Exclude)) {
+                continue;
+            }
+
             let stat = match nix::sys::stat::fstatat(
                 dir_fd,
-                file_name.as_c_str(),
+                file_name.to_owned().as_c_str(),
                 nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
             ) {
                 Ok(stat) => stat,
@@ -444,11 +467,11 @@ impl Archiver {
                 Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
             };
 
-            let match_path = PathBuf::from("/").join(full_path.clone());
-            if self
-                .patterns
-                .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
-                == Some(MatchType::Exclude)
+            if pre_stat_match.is_err()
+                && self
+                    .patterns
+                    .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
+                    == Some(MatchType::Exclude)
             {
                 continue;
             }
@@ -462,7 +485,7 @@ impl Archiver {
             }
 
             file_list.push(FileListEntry {
-                name: file_name,
+                name: file_name.to_owned(),
                 path: full_path,
                 stat,
             });
-- 
2.39.2

Re: [pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[Wolfgang Bumiller]

On Wed, Aug 09, 2023 at 12:19:12PM +0200, Gabriel Goller wrote:
> Added `matches_path()` function, which only matches against the path and returns
> an error if a file_mode pattern is found/needed in the matching list. This is
> useful when we want to check if a file is excluded before running `stat()` on
> the file to get the file_mode (which could fail).
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  src/match_list.rs | 159 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 158 insertions(+), 1 deletion(-)
> 
> diff --git a/src/match_list.rs b/src/match_list.rs
> index c5b14e0..acad328 100644
> --- a/src/match_list.rs
> +++ b/src/match_list.rs
> @@ -1,6 +1,6 @@
>  //! Helpers for include/exclude lists.
> -
>  use bitflags::bitflags;
> +use std::fmt;
>  
>  use crate::PatternFlag;
>  
> @@ -39,6 +39,17 @@ impl Default for MatchFlag {
>      }
>  }
>  
> +#[derive(Debug, PartialEq)]
> +pub struct FileModeRequiredForMatching;

Let's shorten this to just `FileModeRequired` ;-)

> +
> +impl fmt::Display for FileModeRequiredForMatching {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "File mode is required for matching")
> +    }
> +}
> +
> +impl std::error::Error for FileModeRequiredForMatching {}
> +
>  /// A pattern entry. (Glob patterns or literal patterns.)
>  // Note:
>  // For regex we'd likely use the POSIX extended REs via `regexec(3)`, since we're targetting
> @@ -304,12 +315,32 @@ impl MatchEntry {
>  
>          self.matches_path_exact(path)
>      }
> +
> +    /// Check whether the path contains a matching suffix. Returns an error if a file mode is required.
> +    pub fn matches_path<T: AsRef<[u8]>>(
> +        &self,
> +        path: T,
> +    ) -> Result<bool, FileModeRequiredForMatching> {
> +        self.matches_path_do(path.as_ref())
> +    }
> +
> +    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequiredForMatching> {
> +        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
> +            return Err(FileModeRequiredForMatching);
> +        }
> +
> +        Ok(self.matches_path_suffix_do(path))
> +    }
>  }
>  
>  #[doc(hidden)]
>  pub trait MatchListEntry {
>      fn entry_matches(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
>      fn entry_matches_exact(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> +    fn entry_matches_path(
> +        &self,
> +        path: &[u8],
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;

>  }
>  
>  impl MatchListEntry for &'_ MatchEntry {
> @@ -328,6 +359,21 @@ impl MatchListEntry for &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_matches_path(
> +        &self,
> +        path: &[u8],
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> +        if let Ok(b) = self.matches_path(path) {

This can just use `?`, it's the exact same error type after all.
(Also `if let Ok` is generally best avoided since the 'else' branch
discards the error, and if not, it's often a case like this where it can
juse use '?' ;-) ).

Effectively this could be as short as

    Ok(self.matches_path(path)?.then(|| self.match_type()))

> +            if b {

As an additional hint: when you nest ifs around things you can match,
you can just include both cases in the patterns:
    match self.matches_path(path) {
        Ok(true) => Ok(Some(self.match_type())),
        Ok(false) => Ok(None),
        Err(err) => Err(err),
        // where this Err() case already tells you that you can use '?' instead
    }

> +                Ok(Some(self.match_type()))
> +            } else {
> +                Ok(None)
> +            }
> +        } else {
> +            Err(FileModeRequiredForMatching)
> +        }
> +    }
>  }
>  
>  impl MatchListEntry for &'_ &'_ MatchEntry {
> @@ -346,6 +392,21 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_matches_path(
> +        &self,
> +        path: &[u8],
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {

same

> +        if let Ok(b) = self.matches_path(path) {
> +            if b {
> +                Ok(Some(self.match_type()))
> +            } else {
> +                Ok(None)
> +            }
> +        } else {
> +            Err(FileModeRequiredForMatching)
> +        }
> +    }
>  }
>  
>  /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
> @@ -374,6 +435,20 @@ pub trait MatchList {
>      }
>  
>      fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> +
> +    /// Check whether this list contains anything exactly matching the path, returns error if
> +    /// `file_mode` is required for exact matching.
> +    fn matches_path<T: AsRef<[u8]>>(
> +        &self,
> +        path: T,
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> +        self.matches_path_do(path.as_ref())
> +    }
> +
> +    fn matches_path_do(
> +        &self,
> +        path: &[u8],
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
>  }
>  
>  impl<'a, T> MatchList for T
> @@ -408,6 +483,24 @@ where
>  
>          None
>      }
> +
> +    fn matches_path_do(
> +        &self,
> +        path: &[u8],
> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {

Given the amount of tiny match helpers we run through with those 2
traits already I wonder if we should just make a breaking change here
instead and only have the versions with the `Result` while users (or
defaulted helpers in the trait) just pass `file_mode.unwrap_or(!0)`
(since a mode of 0 should match anything ;-) ).

But I don't have any strong feelings about this, so either way is fine
with me.

> +        // This is an &self method on a `T where T: 'a`.
> +        let this: &'a Self = unsafe { std::mem::transmute(self) };
> +
> +        for m in this.into_iter().rev() {
> +            if let Ok(mt) = m.entry_matches_path(path) {

IIRC the intention was actually to immediately fail if we hit a pattern
with a file mode, since we wouldn't be able to tell if it would already
exclude the file, otherwise we really *could* just skip this entirely
and have a failing stat() call just use `Some(!0)` as file mode.

Basically, if the user runs into an inaccessible file they have to
append `--exclude=/that/one/file` to the CLI invocation to fix it,
whereas otherwise they'd still get an error.

So this should just be

    if let Some(mt) = m.entry_matches_path(path)? {
        return Some(mt);
    }


> +                if mt.is_some() {
> +                    return Ok(mt);
> +                }
> +            }
> +        }
> +
> +        Err(FileModeRequiredForMatching)

Also this is wrong. If nothing matches, nothing matches ;-) (just like
in the other matching variants).
Which immediately tells you that skipping the error above doesn't make
much sense :-) )

Re: [pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[Wolfgang Bumiller]

On Fri, Aug 11, 2023 at 10:26:22AM +0200, Wolfgang Bumiller wrote:
> On Wed, Aug 09, 2023 at 12:19:12PM +0200, Gabriel Goller wrote:
> > Added `matches_path()` function, which only matches against the path and returns
> > an error if a file_mode pattern is found/needed in the matching list. This is
> > useful when we want to check if a file is excluded before running `stat()` on
> > the file to get the file_mode (which could fail).
> > 
> > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> > ---
> >  src/match_list.rs | 159 +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 158 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/match_list.rs b/src/match_list.rs
> > index c5b14e0..acad328 100644
> > --- a/src/match_list.rs
> > +++ b/src/match_list.rs
> > @@ -1,6 +1,6 @@
> >  //! Helpers for include/exclude lists.
> > -
> >  use bitflags::bitflags;
> > +use std::fmt;
> >  
> >  use crate::PatternFlag;
> >  
> > @@ -39,6 +39,17 @@ impl Default for MatchFlag {
> >      }
> >  }
> >  
> > +#[derive(Debug, PartialEq)]
> > +pub struct FileModeRequiredForMatching;
> 
> Let's shorten this to just `FileModeRequired` ;-)
> 
> > +
> > +impl fmt::Display for FileModeRequiredForMatching {
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> > +        write!(f, "File mode is required for matching")
> > +    }
> > +}
> > +
> > +impl std::error::Error for FileModeRequiredForMatching {}
> > +
> >  /// A pattern entry. (Glob patterns or literal patterns.)
> >  // Note:
> >  // For regex we'd likely use the POSIX extended REs via `regexec(3)`, since we're targetting
> > @@ -304,12 +315,32 @@ impl MatchEntry {
> >  
> >          self.matches_path_exact(path)
> >      }
> > +
> > +    /// Check whether the path contains a matching suffix. Returns an error if a file mode is required.
> > +    pub fn matches_path<T: AsRef<[u8]>>(
> > +        &self,
> > +        path: T,
> > +    ) -> Result<bool, FileModeRequiredForMatching> {
> > +        self.matches_path_do(path.as_ref())
> > +    }
> > +
> > +    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequiredForMatching> {
> > +        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
> > +            return Err(FileModeRequiredForMatching);
> > +        }
> > +
> > +        Ok(self.matches_path_suffix_do(path))
> > +    }
> >  }
> >  
> >  #[doc(hidden)]
> >  pub trait MatchListEntry {
> >      fn entry_matches(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> >      fn entry_matches_exact(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> > +    fn entry_matches_path(
> > +        &self,
> > +        path: &[u8],
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
> 
> >  }
> >  
> >  impl MatchListEntry for &'_ MatchEntry {
> > @@ -328,6 +359,21 @@ impl MatchListEntry for &'_ MatchEntry {
> >              None
> >          }
> >      }
> > +
> > +    fn entry_matches_path(
> > +        &self,
> > +        path: &[u8],
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> > +        if let Ok(b) = self.matches_path(path) {
> 
> This can just use `?`, it's the exact same error type after all.
> (Also `if let Ok` is generally best avoided since the 'else' branch
> discards the error, and if not, it's often a case like this where it can
> juse use '?' ;-) ).
> 
> Effectively this could be as short as
> 
>     Ok(self.matches_path(path)?.then(|| self.match_type()))
> 
> > +            if b {
> 
> As an additional hint: when you nest ifs around things you can match,
> you can just include both cases in the patterns:
>     match self.matches_path(path) {
>         Ok(true) => Ok(Some(self.match_type())),
>         Ok(false) => Ok(None),
>         Err(err) => Err(err),
>         // where this Err() case already tells you that you can use '?' instead
>     }
> 
> > +                Ok(Some(self.match_type()))
> > +            } else {
> > +                Ok(None)
> > +            }
> > +        } else {
> > +            Err(FileModeRequiredForMatching)
> > +        }
> > +    }
> >  }
> >  
> >  impl MatchListEntry for &'_ &'_ MatchEntry {
> > @@ -346,6 +392,21 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
> >              None
> >          }
> >      }
> > +
> > +    fn entry_matches_path(
> > +        &self,
> > +        path: &[u8],
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> 
> same
> 
> > +        if let Ok(b) = self.matches_path(path) {
> > +            if b {
> > +                Ok(Some(self.match_type()))
> > +            } else {
> > +                Ok(None)
> > +            }
> > +        } else {
> > +            Err(FileModeRequiredForMatching)
> > +        }
> > +    }
> >  }
> >  
> >  /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
> > @@ -374,6 +435,20 @@ pub trait MatchList {
> >      }
> >  
> >      fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
> > +
> > +    /// Check whether this list contains anything exactly matching the path, returns error if
> > +    /// `file_mode` is required for exact matching.
> > +    fn matches_path<T: AsRef<[u8]>>(
> > +        &self,
> > +        path: T,
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> > +        self.matches_path_do(path.as_ref())
> > +    }
> > +
> > +    fn matches_path_do(
> > +        &self,
> > +        path: &[u8],
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
> >  }
> >  
> >  impl<'a, T> MatchList for T
> > @@ -408,6 +483,24 @@ where
> >  
> >          None
> >      }
> > +
> > +    fn matches_path_do(
> > +        &self,
> > +        path: &[u8],
> > +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> 
> Given the amount of tiny match helpers we run through with those 2
> traits already I wonder if we should just make a breaking change here
> instead and only have the versions with the `Result` while users (or
> defaulted helpers in the trait) just pass `file_mode.unwrap_or(!0)`
> (since a mode of 0 should match anything ;-) ).

(of s/of 0/of !0/` of course)

Re: [pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[Wolfgang Bumiller]

On Fri, Aug 11, 2023 at 10:32:59AM +0200, Wolfgang Bumiller wrote:
> On Fri, Aug 11, 2023 at 10:26:22AM +0200, Wolfgang Bumiller wrote:
> > On Wed, Aug 09, 2023 at 12:19:12PM +0200, Gabriel Goller wrote:
(...)
> > 
> > Given the amount of tiny match helpers we run through with those 2
> > traits already I wonder if we should just make a breaking change here
> > instead and only have the versions with the `Result` while users (or
> > defaulted helpers in the trait) just pass `file_mode.unwrap_or(!0)`
> > (since a mode of 0 should match anything ;-) ).
> 
> (of s/of 0/of !0/` of course)

In fact, I'm technically wrong, since the value here just gets masked
and isn't actually a bit field, (similarly wrong will be my comment
further down in my original reply),

However! Since this won't be a valid value, we could still use it and
just special-case this in the `matches_mode()` function.
If, however, we go with an API-breaking route, this is definitely better
implemented as either `enum Mode { Match(u32), Ignore, Fail }` or an
additional "required: bool" parameter... but we need to hand that
through all the layers of helpers...

Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] fix #4380: check if file is excluded before running `stat()`

[Wolfgang Bumiller]

On Wed, Aug 09, 2023 at 12:19:13PM +0200, Gabriel Goller wrote:
> Before running `stat()` we now make a `matches_path()` call, which checks

I was more thinking to use `matches_path()` only if `stat()` actually
fails and then propagate `stat()`'s error (instead of the error that a
file mode is required... ;-) )
That way we wouldn't match twice in the other case.

> if the file/directory is excluded (`matches_path()` tries to match using
> only the path, without the file mode). If the file is not excluded,
> we `stat()` and then check again if the path matches using the file_mode and
> the `matches()` function.
> Added `pathpatterns` crate to local overrides in cargo.toml.
> 
> changes v2:
>  - checking for `read` and `execute` permissions before entering directory,
>    doesn't work because there are a lot of side-effects (executed by
>    different user, AppArmor, SELinux, ...).
> changes v1:
>  - checking for excluded files with `matches()` before executing `stat()`,
>    this doesn't work because we get the file_mode from `stat()` and don't 
>    want to ignore it when matching.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  Cargo.toml                    |  1 +
>  pbs-client/src/pxar/create.rs | 45 ++++++++++++++++++++++++++---------
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 2a05c471..c54bc28b 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -262,6 +262,7 @@ proxmox-rrd.workspace = true
>  
>  #proxmox-apt = { path = "../proxmox-apt" }
>  #proxmox-openid = { path = "../proxmox-openid-rs" }
> +#pathpatterns = {path = "../pathpatterns" }
>  
>  #pxar = { path = "../pxar" }
>  
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 2577cf98..a6d608e7 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -232,7 +232,7 @@ impl Archiver {
>              let entry_counter = self.entry_counter;
>  
>              let old_patterns_count = self.patterns.len();
> -            self.read_pxar_excludes(dir.as_raw_fd())?;
> +            self.read_pxar_excludes(&mut dir)?;
>  
>              let mut file_list = self.generate_directory_file_list(&mut dir, is_root)?;
>  
> @@ -315,8 +315,23 @@ impl Archiver {
>          }
>      }
>  
> -    fn read_pxar_excludes(&mut self, parent: RawFd) -> Result<(), Error> {
> -        let fd = match self.open_file(parent, c_str!(".pxarexclude"), OFlag::O_RDONLY, false)? {
> +    fn read_pxar_excludes(&mut self, parent: &mut Dir) -> Result<(), Error> {
> +        let mut exclude_file_found = false;
> +        for file in parent.iter() {
> +            if file?.file_name().to_str()? == ".pxarexclude" {
> +                exclude_file_found = true;
> +                break;
> +            }
> +        }
> +        if !exclude_file_found {

Why are we adding this check?
It's not necessary.

> +            return Ok(());
> +        }
> +        let fd = match self.open_file(
> +            parent.as_raw_fd(),
> +            c_str!(".pxarexclude"),
> +            OFlag::O_RDONLY,
> +            false,
> +        )? {
>              Some(fd) => fd,
>              None => return Ok(()),

^ This already means the file does not exist.

>          };
> @@ -420,7 +435,7 @@ impl Archiver {
>          for file in dir.iter() {
>              let file = file?;
>  
> -            let file_name = file.file_name().to_owned();
> +            let file_name = file.file_name();
>              let file_name_bytes = file_name.to_bytes();
>              if file_name_bytes == b"." || file_name_bytes == b".." {
>                  continue;
> @@ -434,9 +449,17 @@ impl Archiver {
>              assert_single_path_component(os_file_name)?;
>              let full_path = self.path.join(os_file_name);
>  
> +            let match_path = PathBuf::from("/").join(full_path.clone());
> +            let pre_stat_match = self
> +                .patterns
> +                .matches_path(match_path.as_os_str().as_bytes());

As said above, I'd like this to instead happen in stat's `Err() => {}`
case.

> +            if pre_stat_match == Ok(Some(MatchType::Exclude)) {
> +                continue;
> +            }
> +
>              let stat = match nix::sys::stat::fstatat(
>                  dir_fd,
> -                file_name.as_c_str(),
> +                file_name.to_owned().as_c_str(),
>                  nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
>              ) {
>                  Ok(stat) => stat,
> @@ -444,11 +467,11 @@ impl Archiver {
>                  Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
>              };
>  
> -            let match_path = PathBuf::from("/").join(full_path.clone());
> -            if self
> -                .patterns
> -                .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
> -                == Some(MatchType::Exclude)
> +            if pre_stat_match.is_err()

And then this won't be needed and AFAICT isn't correct with the current
implementation of `matches_path()` of this series since it might have
found a match *after* one of a higher precedence would have already
matched potentially differently.

> +                && self
> +                    .patterns
> +                    .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
> +                    == Some(MatchType::Exclude)
>              {
>                  continue;
>              }
> @@ -462,7 +485,7 @@ impl Archiver {
>              }
>  
>              file_list.push(FileListEntry {
> -                name: file_name,
> +                name: file_name.to_owned(),
>                  path: full_path,
>                  stat,
>              });
> -- 
> 2.39.2

Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] fix #4380: check if file is excluded before running `stat()`

[Gabriel Goller]

On 8/11/23 10:51, Wolfgang Bumiller wrote:
> On Wed, Aug 09, 2023 at 12:19:13PM +0200, Gabriel Goller wrote:
>> Before running `stat()` we now make a `matches_path()` call, which checks
> I was more thinking to use `matches_path()` only if `stat()` actually
> fails and then propagate `stat()`'s error (instead of the error that a
> file mode is required... ;-) )
> That way we wouldn't match twice in the other case.
I used this approach so that we don't `stat` every excluded files as 
well (I think `matches_path` is
probably faster than `stat`). Also we will very rarely make the matching 
twice, because most of the
excluded paths come from the `--exclude` parameter or the `.pxarexclude` 
file, which both don't
allow to set the `file_mode`.
>> if the file/directory is excluded (`matches_path()` tries to match using
>> only the path, without the file mode). If the file is not excluded,
>> we `stat()` and then check again if the path matches using the file_mode and
>> the `matches()` function.
>> Added `pathpatterns` crate to local overrides in cargo.toml.
>>
>> changes v2:
>>   - checking for `read` and `execute` permissions before entering directory,
>>     doesn't work because there are a lot of side-effects (executed by
>>     different user, AppArmor, SELinux, ...).
>> changes v1:
>>   - checking for excluded files with `matches()` before executing `stat()`,
>>     this doesn't work because we get the file_mode from `stat()` and don't
>>     want to ignore it when matching.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   Cargo.toml                    |  1 +
>>   pbs-client/src/pxar/create.rs | 45 ++++++++++++++++++++++++++---------
>>   2 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/Cargo.toml b/Cargo.toml
>> index 2a05c471..c54bc28b 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -262,6 +262,7 @@ proxmox-rrd.workspace = true
>>   
>>   #proxmox-apt = { path = "../proxmox-apt" }
>>   #proxmox-openid = { path = "../proxmox-openid-rs" }
>> +#pathpatterns = {path = "../pathpatterns" }
>>   
>>   #pxar = { path = "../pxar" }
>>   
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index 2577cf98..a6d608e7 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -232,7 +232,7 @@ impl Archiver {
>>               let entry_counter = self.entry_counter;
>>   
>>               let old_patterns_count = self.patterns.len();
>> -            self.read_pxar_excludes(dir.as_raw_fd())?;
>> +            self.read_pxar_excludes(&mut dir)?;
>>   
>>               let mut file_list = self.generate_directory_file_list(&mut dir, is_root)?;
>>   
>> @@ -315,8 +315,23 @@ impl Archiver {
>>           }
>>       }
>>   
>> -    fn read_pxar_excludes(&mut self, parent: RawFd) -> Result<(), Error> {
>> -        let fd = match self.open_file(parent, c_str!(".pxarexclude"), OFlag::O_RDONLY, false)? {
>> +    fn read_pxar_excludes(&mut self, parent: &mut Dir) -> Result<(), Error> {
>> +        let mut exclude_file_found = false;
>> +        for file in parent.iter() {
>> +            if file?.file_name().to_str()? == ".pxarexclude" {
>> +                exclude_file_found = true;
>> +                break;
>> +            }
>> +        }
>> +        if !exclude_file_found {
> Why are we adding this check?
> It's not necessary.

It is not strictly necessary, but the problem is that the function tries 
to open `.pxarexclude` in every directory.
Also directories were we have no access to, which will obviously fail. 
So if we have a directory with limited permissions,
(regardless if a `.pxarexclude` lives in it or not) we will get an error 
`failed to open .pxarexclude file` (even though it
doesn't even exist).

>> +            return Ok(());
>> +        }
>> +        let fd = match self.open_file(
>> +            parent.as_raw_fd(),
>> +            c_str!(".pxarexclude"),
>> +            OFlag::O_RDONLY,
>> +            false,
>> +        )? {
>>               Some(fd) => fd,
>>               None => return Ok(()),
> ^ This already means the file does not exist.
>
>>           };
>> @@ -420,7 +435,7 @@ impl Archiver {
>>           for file in dir.iter() {
>>               let file = file?;
>>   
>> -            let file_name = file.file_name().to_owned();
>> +            let file_name = file.file_name();
>>               let file_name_bytes = file_name.to_bytes();
>>               if file_name_bytes == b"." || file_name_bytes == b".." {
>>                   continue;
>> @@ -434,9 +449,17 @@ impl Archiver {
>>               assert_single_path_component(os_file_name)?;
>>               let full_path = self.path.join(os_file_name);
>>   
>> +            let match_path = PathBuf::from("/").join(full_path.clone());
>> +            let pre_stat_match = self
>> +                .patterns
>> +                .matches_path(match_path.as_os_str().as_bytes());
> As said above, I'd like this to instead happen in stat's `Err() => {}`
> case.
>
>> +            if pre_stat_match == Ok(Some(MatchType::Exclude)) {
>> +                continue;
>> +            }
>> +
>>               let stat = match nix::sys::stat::fstatat(
>>                   dir_fd,
>> -                file_name.as_c_str(),
>> +                file_name.to_owned().as_c_str(),
>>                   nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
>>               ) {
>>                   Ok(stat) => stat,
>> @@ -444,11 +467,11 @@ impl Archiver {
>>                   Err(err) => return Err(err).context(format!("stat failed on {:?}", full_path)),
>>               };
>>   
>> -            let match_path = PathBuf::from("/").join(full_path.clone());
>> -            if self
>> -                .patterns
>> -                .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
>> -                == Some(MatchType::Exclude)
>> +            if pre_stat_match.is_err()
> And then this won't be needed and AFAICT isn't correct with the current
> implementation of `matches_path()` of this series since it might have
> found a match *after* one of a higher precedence would have already
> matched potentially differently.

I'll fix the implementation of `matches_path()` so that it actually 
fails (returns `Err`) when a pattern
with a `file_mode` is encountered (as it was intended). Otherwise this 
should work, as I didn't change
anything (except the preceding `matches_path()`).

>> +                && self
>> +                    .patterns
>> +                    .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode))
>> +                    == Some(MatchType::Exclude)
>>               {
>>                   continue;
>>               }
>> @@ -462,7 +485,7 @@ impl Archiver {
>>               }
>>   
>>               file_list.push(FileListEntry {
>> -                name: file_name,
>> +                name: file_name.to_owned(),
>>                   path: full_path,
>>                   stat,
>>               });
>> -- 
>> 2.39.2

Re: [pbs-devel] [PATCH pathpatterns] match_list: added `matches_path()` function, which matches only the path

[Gabriel Goller]

On 8/11/23 10:26, Wolfgang Bumiller wrote:

> On Wed, Aug 09, 2023 at 12:19:12PM +0200, Gabriel Goller wrote:
>> Added `matches_path()` function, which only matches against the path and returns
>> an error if a file_mode pattern is found/needed in the matching list. This is
>> useful when we want to check if a file is excluded before running `stat()` on
>> the file to get the file_mode (which could fail).
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   src/match_list.rs | 159 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 158 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/match_list.rs b/src/match_list.rs
>> index c5b14e0..acad328 100644
>> --- a/src/match_list.rs
>> +++ b/src/match_list.rs
>> @@ -1,6 +1,6 @@
>>   //! Helpers for include/exclude lists.
>> -
>>   use bitflags::bitflags;
>> +use std::fmt;
>>   
>>   use crate::PatternFlag;
>>   
>> @@ -39,6 +39,17 @@ impl Default for MatchFlag {
>>       }
>>   }
>>   
>> +#[derive(Debug, PartialEq)]
>> +pub struct FileModeRequiredForMatching;
> Let's shorten this to just `FileModeRequired` ;-)
Agree
>> +
>> +impl fmt::Display for FileModeRequiredForMatching {
>> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> +        write!(f, "File mode is required for matching")
>> +    }
>> +}
>> +
>> +impl std::error::Error for FileModeRequiredForMatching {}
>> +
>>   /// A pattern entry. (Glob patterns or literal patterns.)
>>   // Note:
>>   // For regex we'd likely use the POSIX extended REs via `regexec(3)`, since we're targetting
>> @@ -304,12 +315,32 @@ impl MatchEntry {
>>   
>>           self.matches_path_exact(path)
>>       }
>> +
>> +    /// Check whether the path contains a matching suffix. Returns an error if a file mode is required.
>> +    pub fn matches_path<T: AsRef<[u8]>>(
>> +        &self,
>> +        path: T,
>> +    ) -> Result<bool, FileModeRequiredForMatching> {
>> +        self.matches_path_do(path.as_ref())
>> +    }
>> +
>> +    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequiredForMatching> {
>> +        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
>> +            return Err(FileModeRequiredForMatching);
>> +        }
>> +
>> +        Ok(self.matches_path_suffix_do(path))
>> +    }
>>   }
>>   
>>   #[doc(hidden)]
>>   pub trait MatchListEntry {
>>       fn entry_matches(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
>>       fn entry_matches_exact(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
>> +    fn entry_matches_path(
>> +        &self,
>> +        path: &[u8],
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
>>   }
>>   
>>   impl MatchListEntry for &'_ MatchEntry {
>> @@ -328,6 +359,21 @@ impl MatchListEntry for &'_ MatchEntry {
>>               None
>>           }
>>       }
>> +
>> +    fn entry_matches_path(
>> +        &self,
>> +        path: &[u8],
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
>> +        if let Ok(b) = self.matches_path(path) {
> This can just use `?`, it's the exact same error type after all.
> (Also `if let Ok` is generally best avoided since the 'else' branch
> discards the error, and if not, it's often a case like this where it can
> juse use '?' ;-) ).
>
> Effectively this could be as short as
>
>      Ok(self.matches_path(path)?.then(|| self.match_type()))
>
>> +            if b {
> As an additional hint: when you nest ifs around things you can match,
> you can just include both cases in the patterns:
>      match self.matches_path(path) {
>          Ok(true) => Ok(Some(self.match_type())),
>          Ok(false) => Ok(None),
>          Err(err) => Err(err),
>          // where this Err() case already tells you that you can use '?' instead
>      }
>
>> +                Ok(Some(self.match_type()))
>> +            } else {
>> +                Ok(None)
>> +            }
>> +        } else {
>> +            Err(FileModeRequiredForMatching)
>> +        }
>> +    }
>>   }
>>   
>>   impl MatchListEntry for &'_ &'_ MatchEntry {
>> @@ -346,6 +392,21 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
>>               None
>>           }
>>       }
>> +
>> +    fn entry_matches_path(
>> +        &self,
>> +        path: &[u8],
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> same
>
>> +        if let Ok(b) = self.matches_path(path) {
>> +            if b {
>> +                Ok(Some(self.match_type()))
>> +            } else {
>> +                Ok(None)
>> +            }
>> +        } else {
>> +            Err(FileModeRequiredForMatching)
>> +        }
>> +    }
>>   }
>>   
>>   /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
>> @@ -374,6 +435,20 @@ pub trait MatchList {
>>       }
>>   
>>       fn matches_exact_do(&self, path: &[u8], file_mode: Option<u32>) -> Option<MatchType>;
>> +
>> +    /// Check whether this list contains anything exactly matching the path, returns error if
>> +    /// `file_mode` is required for exact matching.
>> +    fn matches_path<T: AsRef<[u8]>>(
>> +        &self,
>> +        path: T,
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
>> +        self.matches_path_do(path.as_ref())
>> +    }
>> +
>> +    fn matches_path_do(
>> +        &self,
>> +        path: &[u8],
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching>;
>>   }
>>   
>>   impl<'a, T> MatchList for T
>> @@ -408,6 +483,24 @@ where
>>   
>>           None
>>       }
>> +
>> +    fn matches_path_do(
>> +        &self,
>> +        path: &[u8],
>> +    ) -> Result<Option<MatchType>, FileModeRequiredForMatching> {
> Given the amount of tiny match helpers we run through with those 2
> traits already I wonder if we should just make a breaking change here
> instead and only have the versions with the `Result` while users (or
> defaulted helpers in the trait) just pass `file_mode.unwrap_or(!0)`
> (since a mode of 0 should match anything ;-) ).
>
> But I don't have any strong feelings about this, so either way is fine
> with me.

Hmm, I don't get what you mean by the `versions with the Result`?
Do you mean we should just modify the `matches()` and `matches_exact()` 
function to
return a `Result` if the mode is `ANY_FILE_TYPE` and we need one (a file 
mode) to match?

>> +        // This is an &self method on a `T where T: 'a`.
>> +        let this: &'a Self = unsafe { std::mem::transmute(self) };
>> +
>> +        for m in this.into_iter().rev() {
>> +            if let Ok(mt) = m.entry_matches_path(path) {
> IIRC the intention was actually to immediately fail if we hit a pattern
> with a file mode, since we wouldn't be able to tell if it would already
> exclude the file, otherwise we really *could* just skip this entirely
> and have a failing stat() call just use `Some(!0)` as file mode.
>
> Basically, if the user runs into an inaccessible file they have to
> append `--exclude=/that/one/file` to the CLI invocation to fix it,
> whereas otherwise they'd still get an error.
>
> So this should just be
>
>      if let Some(mt) = m.entry_matches_path(path)? {
>          return Some(mt);
>      }

Hmm, my solution would have been to traverse all the patterns with a 
`ANY_FILE_TYPE` mode and
if there is a match, nice, return. If there is no match, check if there 
are other patterns without
the `ANY_FILE_TYPE` mode (then we need to fail because we don't know the 
`file_mode` yet...) or
return nothing if there are no patterns left. I don't know if this is 
the correct approach though...

>> +                if mt.is_some() {
>> +                    return Ok(mt);
>> +                }
>> +            }
>> +        }
>> +
>> +        Err(FileModeRequiredForMatching)
> Also this is wrong. If nothing matches, nothing matches ;-) (just like
> in the other matching variants).
> Which immediately tells you that skipping the error above doesn't make
> much sense :-) )
Yes, you are right, corrected it.