public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary
@ 2023-08-16 14:03 Gabriel Goller
  2023-08-16 14:03 ` [pbs-devel] [PATCH proxmox-backup v4] fix #4380: check if file is excluded before running `stat()` Gabriel Goller
  2023-08-17  9:01 ` [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Wolfgang Bumiller
  0 siblings, 2 replies; 3+ messages in thread
From: Gabriel Goller @ 2023-08-16 14:03 UTC (permalink / raw)
  To: pbs-devel

Added `matches_mode_lazy()` function, which takes a closure that should return the
`file_mode`. The function will go through the patterns and match by path only
until it finds a pattern which does not have `MatchFlag::ANY_FILE_TYPE`, in case
it will call the closure, which will return a `file_mode`. This will ensure that
the closure (which in our case executes `stat()`) will only be run once(at most),
every pattern will only be processed once and that the order of the patterns
will be respected.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

changes v2:
 - updated the `matches_path()` function to take a closure and renamed 
   it to `matches_mode_lazy()`. This allows us to only match once, so we 
   don't need to match before and after `stat()` (without and with 
   `file_mode`) anymore.

changes v1:
 - added `matches_path()` function, which matches by path only and returns an
   error if the `file_mode` is required.

 src/match_list.rs | 203 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 202 insertions(+), 1 deletion(-)

diff --git a/src/match_list.rs b/src/match_list.rs
index c5b14e0..9914130 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::{error::Error, fmt};
 
 use crate::PatternFlag;
 
@@ -39,6 +39,17 @@ impl Default for MatchFlag {
     }
 }
 
+#[derive(Debug, PartialEq)]
+pub struct FileModeRequired;
+
+impl fmt::Display for FileModeRequired {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "File mode is required for matching")
+    }
+}
+
+impl Error for FileModeRequired {}
+
 /// 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,26 @@ 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, FileModeRequired> {
+        self.matches_path_do(path.as_ref())
+    }
+
+    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequired> {
+        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
+            return Err(FileModeRequired);
+        }
+
+        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>, FileModeRequired>;
 }
 
 impl MatchListEntry for &'_ MatchEntry {
@@ -328,6 +353,15 @@ impl MatchListEntry for &'_ MatchEntry {
             None
         }
     }
+
+    fn entry_matches_path(&self, path: &[u8]) -> Result<Option<MatchType>, FileModeRequired> {
+        let matches = self.matches_path(path)?;
+        if matches {
+            Ok(Some(self.match_type()))
+        } else {
+            Ok(None)
+        }
+    }
 }
 
 impl MatchListEntry for &'_ &'_ MatchEntry {
@@ -346,6 +380,15 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
             None
         }
     }
+
+    fn entry_matches_path(&self, path: &[u8]) -> Result<Option<MatchType>, FileModeRequired> {
+        let matches = self.matches_path(path)?;
+        if matches {
+            Ok(Some(self.match_type()))
+        } else {
+            Ok(None)
+        }
+    }
 }
 
 /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
@@ -374,6 +417,22 @@ 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 and cannot be retrieved using closure.
+    fn matches_mode_lazy<T: AsRef<[u8]>, E: Error, U: FnMut() -> Result<u32, E>>(
+        &self,
+        path: T,
+        get_file_mode: U,
+    ) -> Result<Option<MatchType>, E> {
+        self.matches_mode_lazy_do(path.as_ref(), get_file_mode)
+    }
+
+    fn matches_mode_lazy_do<E: Error, T: FnMut() -> Result<u32, E>>(
+        &self,
+        path: &[u8],
+        get_file_mode: T,
+    ) -> Result<Option<MatchType>, E>;
 }
 
 impl<'a, T> MatchList for T
@@ -408,6 +467,39 @@ where
 
         None
     }
+
+    fn matches_mode_lazy_do<E: Error, U: FnMut() -> Result<u32, E>>(
+        &self,
+        path: &[u8],
+        mut get_file_mode: U,
+    ) -> Result<Option<MatchType>, E> {
+        // This is an &self method on a `T where T: 'a`.
+        let this: &'a Self = unsafe { std::mem::transmute(self) };
+
+        let mut file_mode: Option<u32> = None;
+        for m in this.into_iter().rev() {
+            if file_mode.is_none() {
+                match m.entry_matches_path(path) {
+                    Ok(mt) => {
+                        if mt.is_some() {
+                            return Ok(mt);
+                        }
+                    }
+                    Err(_) => match get_file_mode() {
+                        Ok(x) => file_mode = Some(x),
+                        Err(e) => return Err(e),
+                    },
+                }
+            }
+            if file_mode.is_some() {
+                let full_match = m.entry_matches(path, file_mode);
+                if full_match.is_some() {
+                    return Ok(full_match);
+                }
+            }
+        }
+        Ok(None)
+    }
 }
 
 #[test]
@@ -530,3 +622,112 @@ fn test_path_relativity() {
     assert_eq!(matchlist.matches("foo/slash", None), None);
     assert_eq!(matchlist.matches("foo/slash-a", None), None);
 }
+
+#[test]
+fn matches_path() {
+    use crate::Pattern;
+
+    #[derive(fmt::Debug, PartialEq)]
+    struct ExampleErr {}
+    impl fmt::Display for ExampleErr {
+        fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
+            todo!()
+        }
+    }
+    impl Error for ExampleErr {}
+
+    let matchlist = vec![
+        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude),
+        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude),
+    ];
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Err(ExampleErr {})),
+        Ok(Some(MatchType::Exclude))
+    );
+    let mut _test = MatchFlag::ANY_FILE_TYPE;
+    assert_eq!(
+        matchlist.matches_mode_lazy("bhshdf", || {
+            _test = MatchFlag::MATCH_DIRECTORIES;
+            Ok::<u32, ExampleErr>(libc::S_IFDIR)
+        }),
+        Ok(Some(MatchType::Exclude))
+    );
+
+    let matchlist = vec![
+        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
+            .flags(MatchFlag::MATCH_DIRECTORIES),
+        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude),
+    ];
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Err(ExampleErr {})),
+        Err(ExampleErr {})
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("bhshdf", || Err(ExampleErr {})),
+        Ok(Some(MatchType::Exclude))
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Ok::<u32, ExampleErr>(libc::S_IFDIR)),
+        Ok(Some(MatchType::Exclude))
+    );
+
+    let matchlist = vec![
+        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Include),
+        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
+            .flags(MatchFlag::MATCH_DIRECTORIES),
+    ];
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Err(ExampleErr {})),
+        Err(ExampleErr {})
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("bhshdf", || Err(ExampleErr {})),
+        Err(ExampleErr {})
+    );
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Ok::<u32, ExampleErr>(libc::S_IFDIR)),
+        Ok(Some(MatchType::Exclude))
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("bhshdf", || Err(ExampleErr {})),
+        Err(ExampleErr {})
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("bbb", || Ok::<u32, ExampleErr>(libc::S_IFDIR)),
+        Ok(Some(MatchType::Include))
+    );
+
+    let matchlist = vec![
+        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
+            .flags(MatchFlag::MATCH_DIRECTORIES),
+    ];
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("bbb", || Ok::<u32, ExampleErr>(libc::S_IFDIR)),
+        Ok(None)
+    );
+
+    let matchlist = vec![
+        MatchEntry::new(Pattern::path("a*").unwrap(), MatchType::Exclude)
+            .flags(MatchFlag::MATCH_DIRECTORIES),
+        MatchEntry::new(Pattern::path("b*").unwrap(), MatchType::Exclude)
+            .flags(MatchFlag::MATCH_REGULAR_FILES),
+    ];
+
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Ok::<u32, ExampleErr>(libc::S_IFDIR)),
+        Ok(Some(MatchType::Exclude))
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("ahsjdj", || Ok::<u32, ExampleErr>(libc::S_IFREG)),
+        Ok(None)
+    );
+    assert_eq!(
+        matchlist.matches_mode_lazy("bhsjdj", || Ok::<u32, ExampleErr>(libc::S_IFREG)),
+        Ok(Some(MatchType::Exclude))
+    );
+}
-- 
2.39.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pbs-devel] [PATCH proxmox-backup v4] fix #4380: check if file is excluded before running `stat()`
  2023-08-16 14:03 [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Gabriel Goller
@ 2023-08-16 14:03 ` Gabriel Goller
  2023-08-17  9:01 ` [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Wolfgang Bumiller
  1 sibling, 0 replies; 3+ messages in thread
From: Gabriel Goller @ 2023-08-16 14:03 UTC (permalink / raw)
  To: pbs-devel

Passed a closure with the `stat()` function call to `match_mode_lazy()`. This
will traverse through all patterns and try to match using the path only, if a
`file_mode` is needed, it will run the closure. This means that if we exclude
a file with the `MatchType::ANY_FILE_TYPE`, we will skip it without running
`stat()` on it.
Added `pathpatterns` crate to local overrides in cargo.toml.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

changes v3:
 - match only by path and exclude the matched files, the run `stat()` and
   match again, this time using the `file_mode`. This will match everything 
   twice in the worst case, which is not optimal.
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.

 Cargo.toml                    |  1 +
 pbs-client/src/pxar/create.rs | 68 +++++++++++++++++++++++++----------
 2 files changed, 50 insertions(+), 19 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..7027a7d2 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -21,7 +21,6 @@ use pxar::Metadata;
 
 use proxmox_io::vec;
 use proxmox_lang::c_str;
-use proxmox_sys::error::SysError;
 use proxmox_sys::fs::{self, acl, xattr};
 
 use pbs_datastore::catalog::BackupCatalogWriter;
@@ -420,7 +419,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,25 +433,56 @@ impl Archiver {
             assert_single_path_component(os_file_name)?;
             let full_path = self.path.join(os_file_name);
 
-            let stat = match nix::sys::stat::fstatat(
-                dir_fd,
-                file_name.as_c_str(),
-                nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
-            ) {
-                Ok(stat) => stat,
-                Err(ref err) if err.not_found() => continue,
-                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)
-            {
+
+            let mut stat_results: Option<FileStat> = None;
+
+            let stat =
+                self.patterns
+                    .matches_mode_lazy(match_path.as_os_str().as_bytes(), || {
+                        match nix::sys::stat::fstatat(
+                            dir_fd,
+                            file_name.to_owned().as_c_str(),
+                            nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                        ) {
+                            Ok(stat) => {
+                                stat_results = Some(stat);
+                                Ok(stat.st_mode)
+                            }
+                            Err(e) => Err(e),
+                        }
+                    })
+                    .map_err(|err| {
+                        anyhow::Error::new(err).context(format!(
+                            "stat failed on {:?} with {:?}",
+                            full_path,
+                            err.desc()
+                        ))
+                    })?;
+
+            if stat == Some(MatchType::Exclude) {
                 continue;
             }
 
+            if stat_results.is_none() {
+                match nix::sys::stat::fstatat(
+                    dir_fd,
+                    file_name.to_owned().as_c_str(),
+                    nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                ) {
+                    Ok(stat) => {
+                        stat_results = Some(stat);
+                    }
+                    Err(err) => {
+                        return Err(err).context(format!(
+                            "stat failed on {:?} with {:?}",
+                            full_path,
+                            err.desc()
+                        ));
+                    }
+                }
+            }
+
             self.entry_counter += 1;
             if self.entry_counter > self.entry_limit {
                 bail!(
@@ -462,9 +492,9 @@ impl Archiver {
             }
 
             file_list.push(FileListEntry {
-                name: file_name,
+                name: file_name.to_owned(),
                 path: full_path,
-                stat,
+                stat: stat_results.unwrap(),
             });
         }
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary
  2023-08-16 14:03 [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Gabriel Goller
  2023-08-16 14:03 ` [pbs-devel] [PATCH proxmox-backup v4] fix #4380: check if file is excluded before running `stat()` Gabriel Goller
@ 2023-08-17  9:01 ` Wolfgang Bumiller
  1 sibling, 0 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2023-08-17  9:01 UTC (permalink / raw)
  To: Gabriel Goller; +Cc: pbs-devel

Getting there :-)
I'd still like a few changes.

On Wed, Aug 16, 2023 at 04:03:29PM +0200, Gabriel Goller wrote:
> Added `matches_mode_lazy()` function, which takes a closure that should return the
> `file_mode`. The function will go through the patterns and match by path only
> until it finds a pattern which does not have `MatchFlag::ANY_FILE_TYPE`, in case
> it will call the closure, which will return a `file_mode`. This will ensure that
> the closure (which in our case executes `stat()`) will only be run once(at most),
> every pattern will only be processed once and that the order of the patterns
> will be respected.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> changes v2:
>  - updated the `matches_path()` function to take a closure and renamed 
>    it to `matches_mode_lazy()`. This allows us to only match once, so we 
>    don't need to match before and after `stat()` (without and with 
>    `file_mode`) anymore.
> 
> changes v1:
>  - added `matches_path()` function, which matches by path only and returns an
>    error if the `file_mode` is required.
> 
>  src/match_list.rs | 203 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 202 insertions(+), 1 deletion(-)
> 
> diff --git a/src/match_list.rs b/src/match_list.rs
> index c5b14e0..9914130 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::{error::Error, fmt};
>  
>  use crate::PatternFlag;
>  
> @@ -39,6 +39,17 @@ impl Default for MatchFlag {
>      }
>  }
>  
> +#[derive(Debug, PartialEq)]
> +pub struct FileModeRequired;
> +
> +impl fmt::Display for FileModeRequired {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "File mode is required for matching")
> +    }
> +}
> +
> +impl Error for FileModeRequired {}
> +
>  /// 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,26 @@ impl MatchEntry {
>  
>          self.matches_path_exact(path)
>      }
> +

These can be dropped, because we can actually avoid the need to call any
`matches` functions that would require a mode if we don't actually have
a mode.

For this purpose, `MatchEntry` should just have a

    pub fn needs_file_mode(&self) -> bool;

(also utilized in its `fn matches_mode()` for less duplicate bitflag
handling code)

> +    /// 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, FileModeRequired> {
> +        self.matches_path_do(path.as_ref())
> +    }
> +
> +    fn matches_path_do(&self, path: &[u8]) -> Result<bool, FileModeRequired> {
> +        if !self.flags.contains(MatchFlag::ANY_FILE_TYPE) {
> +            return Err(FileModeRequired);
> +        }
> +
> +        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>, FileModeRequired>;

Instead of this, we should expose
    fn needs_file_mode(&self) -> bool
in this trait.

>  }
>  
>  impl MatchListEntry for &'_ MatchEntry {
> @@ -328,6 +353,15 @@ impl MatchListEntry for &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_matches_path(&self, path: &[u8]) -> Result<Option<MatchType>, FileModeRequired> {
> +        let matches = self.matches_path(path)?;
> +        if matches {
> +            Ok(Some(self.match_type()))
> +        } else {
> +            Ok(None)
> +        }
> +    }
>  }
>  
>  impl MatchListEntry for &'_ &'_ MatchEntry {
> @@ -346,6 +380,15 @@ impl MatchListEntry for &'_ &'_ MatchEntry {
>              None
>          }
>      }
> +
> +    fn entry_matches_path(&self, path: &[u8]) -> Result<Option<MatchType>, FileModeRequired> {
> +        let matches = self.matches_path(path)?;
> +        if matches {
> +            Ok(Some(self.match_type()))
> +        } else {
> +            Ok(None)
> +        }
> +    }
>  }
>  
>  /// This provides [`matches`](MatchList::matches) and [`matches_exact`](MatchList::matches_exact)
> @@ -374,6 +417,22 @@ pub trait MatchList {
>      }

And here would be the main API break.
All of the methods here in `MatchList` should act as lazy instead of
having this as a new method.

The only downside is that this means fixing up all the test cases... ;-)

>  
>      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 and cannot be retrieved using closure.
> +    fn matches_mode_lazy<T: AsRef<[u8]>, E: Error, U: FnMut() -> Result<u32, E>>(

Style nit: with large trait bounds, please move them into a `where`
clause instead (same in the other cases where you add these).

> +        &self,
> +        path: T,
> +        get_file_mode: U,
> +    ) -> Result<Option<MatchType>, E> {
> +        self.matches_mode_lazy_do(path.as_ref(), get_file_mode)
> +    }
> +
> +    fn matches_mode_lazy_do<E: Error, T: FnMut() -> Result<u32, E>>(
> +        &self,
> +        path: &[u8],
> +        get_file_mode: T,
> +    ) -> Result<Option<MatchType>, E>;
>  }
>  
>  impl<'a, T> MatchList for T
> @@ -408,6 +467,39 @@ where
>  
>          None
>      }
> +
> +    fn matches_mode_lazy_do<E: Error, U: FnMut() -> Result<u32, E>>(

`FnOnce` should suffice, to guarantee at the API level that we only call
it once.

You can make this work by moving it into an `Option` first:

    let mut get_file_mode = Some(get_file_mode);
    let mut file_mode = None;
    for m in ... {
        if file_mode.is_none() && m.needs_file_mode() {
            // unwrap: we always have get_file_mode XOR file_mode as Some()
            file_mode = Some((get_file_mode.take().unwrap())()?);
        }
        if let Some(mt) = m.entry_matches(path, file_mode) {
            return Ok(Some(mt));
        }
    }

Note that we just call the regular `entry_matches`, since given `None`
as file mode it simply won't bother with the file mode, but at the same
time, if the pattern does *want* a file mode, we try to fetch it.

Now, as a bonus, we could also make `None` work again as a parameter for
these methods by replacing the `FnOnce` parameter with a new small
custom trait:

    pub trait GetFileMode {
        type Error;
        fn get(self) -> Result<u32, Self::Error>;
    }

With this being our own trait, we can have a generic impl along with a
one for `Option<u32>`:

    impl<T> GetFileMode for T
    where
        T: FnOnce() -> Result<u32, E>,
    {
        type Error = E;
        fn get(self) -> Result<u32, Self::Error> {
            return self();
        }
    }

    impl GetFileMode for Option<u32> {
        type Error = FileModeRequired;
        fn get(self) -> Result<u32, Self::Error> {
            self.ok_or(FileModeRequired)
        }
    }

^ otherwise we also won't need the `FileModeRequired` error type ;-)

This will also make adapting the test cases much easier (only the `Ok()`
around the result types needs to be added, no closures for the modes).


> +        &self,
> +        path: &[u8],
> +        mut get_file_mode: U,
> +    ) -> Result<Option<MatchType>, E> {
> +        // This is an &self method on a `T where T: 'a`.
> +        let this: &'a Self = unsafe { std::mem::transmute(self) };
> +
> +        let mut file_mode: Option<u32> = None;
> +        for m in this.into_iter().rev() {
> +            if file_mode.is_none() {
> +                match m.entry_matches_path(path) {
> +                    Ok(mt) => {
> +                        if mt.is_some() {
> +                            return Ok(mt);
> +                        }
> +                    }
> +                    Err(_) => match get_file_mode() {
> +                        Ok(x) => file_mode = Some(x),
> +                        Err(e) => return Err(e),
> +                    },
> +                }
> +            }
> +            if file_mode.is_some() {
> +                let full_match = m.entry_matches(path, file_mode);
> +                if full_match.is_some() {
> +                    return Ok(full_match);
> +                }
> +            }
> +        }
> +        Ok(None)
> +    }
>  }
>  
>  #[test]




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-08-17  9:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-16 14:03 [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Gabriel Goller
2023-08-16 14:03 ` [pbs-devel] [PATCH proxmox-backup v4] fix #4380: check if file is excluded before running `stat()` Gabriel Goller
2023-08-17  9:01 ` [pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary Wolfgang Bumiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal