[pbs-devel] [PATCH pathpatterns v2] match_list: added `matches_mode_lazy()`, which only retrieves file mode if necessary
Wolfgang Bumiller
w.bumiller at proxmox.com
Thu Aug 17 11:01:26 CEST 2023
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 at 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]
More information about the pbs-devel
mailing list