-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fail on permissions flag #551
base: main
Are you sure you want to change the base?
Changes from 5 commits
5496ba5
c7c8d0e
e63a681
6b22759
d90f552
e315d15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,5 @@ | |
tests/last-fails | ||
tests/last-run.log | ||
.cargo | ||
.idea/ | ||
*.iml | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,9 @@ pub struct Config { | |
|
||
/// See [BackendConfig::with_compare_contents] | ||
compare_contents: bool, | ||
|
||
/// See [BackendConfig::with_fail_on_no_permissions] | ||
fail_on_no_permissions: bool, | ||
} | ||
|
||
impl Config { | ||
|
@@ -94,13 +97,29 @@ impl Config { | |
pub fn compare_contents(&self) -> bool { | ||
self.compare_contents | ||
} | ||
|
||
/// For the [INotifyWatcher](crate::INotifyWatcher) backend. | ||
/// | ||
/// This flag sets whether inotify should fail when the user has no permissions on a subfolder. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this isn't actually true - my guess is you tested this on linux where we have to subscribe to each system ? |
||
/// | ||
/// On by default. | ||
pub fn with_fail_on_no_permissions(mut self, fail_on_no_permissions: bool) -> Self { | ||
self.fail_on_no_permissions = fail_on_no_permissions; | ||
self | ||
} | ||
|
||
/// Returns current setting | ||
pub fn fail_on_no_permissions(&self) -> bool { | ||
self.fail_on_no_permissions | ||
} | ||
} | ||
|
||
impl Default for Config { | ||
fn default() -> Self { | ||
Self { | ||
poll_interval: Some(Duration::from_secs(30)), | ||
compare_contents: false, | ||
fail_on_no_permissions: true, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,13 @@ use crate::{bounded, unbounded, BoundSender, Receiver, Sender}; | |
use inotify as inotify_sys; | ||
use inotify_sys::{EventMask, Inotify, WatchDescriptor, WatchMask}; | ||
use std::collections::HashMap; | ||
use std::env; | ||
use std::ffi::OsStr; | ||
use std::fs::metadata; | ||
use std::os::unix::io::AsRawFd; | ||
use std::path::{Path, PathBuf}; | ||
use std::sync::Arc; | ||
use std::thread; | ||
use std::{env, io}; | ||
use walkdir::WalkDir; | ||
|
||
const INOTIFY: mio::Token = mio::Token(0); | ||
|
@@ -40,6 +40,7 @@ struct EventLoop { | |
watches: HashMap<PathBuf, (WatchDescriptor, WatchMask, bool, bool)>, | ||
paths: HashMap<WatchDescriptor, PathBuf>, | ||
rename_event: Option<Event>, | ||
fail_on_no_permissions: bool, | ||
} | ||
|
||
/// Watcher implementation based on inotify | ||
|
@@ -90,7 +91,11 @@ fn remove_watch_by_event( | |
} | ||
|
||
impl EventLoop { | ||
pub fn new(inotify: Inotify, event_handler: Box<dyn EventHandler>) -> Result<Self> { | ||
pub fn new( | ||
inotify: Inotify, | ||
event_handler: Box<dyn EventHandler>, | ||
fail_on_no_permissions: bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer passing a config like this via a wrapper (a struct) instead, otherwise args could be too long and we have to expect breaking changes each time we modify. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, please pass the config down if possible |
||
) -> Result<Self> { | ||
let (event_loop_tx, event_loop_rx) = unbounded::<EventLoopMsg>(); | ||
let poll = mio::Poll::new()?; | ||
|
||
|
@@ -112,6 +117,7 @@ impl EventLoop { | |
watches: HashMap::new(), | ||
paths: HashMap::new(), | ||
rename_event: None, | ||
fail_on_no_permissions, | ||
}; | ||
Ok(event_loop) | ||
} | ||
|
@@ -307,9 +313,11 @@ impl EventLoop { | |
} | ||
} | ||
None => { | ||
log::trace!("No patch for DELETE_SELF event, may be a bug?"); | ||
log::trace!( | ||
"No patch for DELETE_SELF event, may be a bug?" | ||
); | ||
RemoveKind::Other | ||
}, | ||
} | ||
}; | ||
evs.push( | ||
Event::new(EventKind::Remove(remove_kind)) | ||
|
@@ -396,7 +404,20 @@ impl EventLoop { | |
.into_iter() | ||
.filter_map(filter_dir) | ||
{ | ||
self.add_single_watch(entry.path().to_path_buf(), is_recursive, watch_self)?; | ||
if let Err(e) = | ||
self.add_single_watch(entry.path().to_path_buf(), is_recursive, watch_self) | ||
{ | ||
match &e.kind { | ||
ErrorKind::Io(io) if io.kind() == io::ErrorKind::PermissionDenied => { | ||
// If this is the root folder we want to fail on permission error even if | ||
// fail_on_permissions_for_subfolders flag is not set | ||
if watch_self || self.fail_on_no_permissions { | ||
return Err(e); | ||
} | ||
} | ||
_ => return Err(e), | ||
} | ||
} | ||
watch_self = false; | ||
} | ||
|
||
|
@@ -514,9 +535,12 @@ fn filter_dir(e: walkdir::Result<walkdir::DirEntry>) -> Option<walkdir::DirEntry | |
} | ||
|
||
impl INotifyWatcher { | ||
fn from_event_handler(event_handler: Box<dyn EventHandler>) -> Result<Self> { | ||
fn from_event_handler( | ||
event_handler: Box<dyn EventHandler>, | ||
fail_on_no_permissions: bool, | ||
) -> Result<Self> { | ||
let inotify = Inotify::init()?; | ||
let event_loop = EventLoop::new(inotify, event_handler)?; | ||
let event_loop = EventLoop::new(inotify, event_handler, fail_on_no_permissions)?; | ||
let channel = event_loop.event_loop_tx.clone(); | ||
let waker = event_loop.event_loop_waker.clone(); | ||
event_loop.run(); | ||
|
@@ -558,8 +582,8 @@ impl INotifyWatcher { | |
|
||
impl Watcher for INotifyWatcher { | ||
/// Create a new watcher. | ||
fn new<F: EventHandler>(event_handler: F, _config: Config) -> Result<Self> { | ||
Self::from_event_handler(Box::new(event_handler)) | ||
fn new<F: EventHandler>(event_handler: F, config: Config) -> Result<Self> { | ||
Self::from_event_handler(Box::new(event_handler), config.fail_on_no_permissions()) | ||
} | ||
|
||
fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add them to your global gitignore instead? Accepting any IDEs makes our gitignore fat and hard to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this