Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support watching the same inode through multiple paths. #573

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support watching the same inode through multiple paths. #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions notify-types/src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,13 @@ pub struct Event {
/// creation, they should all go in this `Vec`. Otherwise, using the `Tracker` attr may be more
/// appropriate.
///
/// The order of the paths is likely to be significant! For example, renames where both ends of
/// the name change are known will have the "source" path first, and the "target" path last.
/// The order of the paths is likely to be significant! Furthermore, the paths might be part of
/// different "groups". For example, renames where both ends of the name change are known will
/// have the "source" paths first, followed by the "target" paths. These two sets of paths are
/// in distinct groups.
///
/// If there are different path groups, the `path_group_split_index` attribute indicates the
/// first index with paths in the second group.
pub paths: Vec<PathBuf>,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a behavioral change, we probably want some kind of API change so users explicitly decide how to handle multiple paths in one group, or this newly introduced concept of 2 different groups.

I didn't want to break this into two separate vectors, as to not increase the size of the Event struct. Maybe we should make paths private and have a fn path_groups(&self) -> (&[PathBuf], Option<&[PathBuf]>) method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about instead of having the api on the attrs interact directly with the fields, there's a more intuitive API on Event like renamed_from_paths() -> Option<&[Path]> and renamed_to_paths() -> Option<&[Path]> that interprets the kind/paths/attr information?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a good way to only expose these APIs on renames only. Are you proposing to check the event kind and only return Some in case of a rename?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was, yes.


// "What should be in the struct" and "what can go in the attrs" is an interesting question.
Expand Down Expand Up @@ -400,6 +405,19 @@ struct EventAttributesInner {
)]
source: Option<String>,

/// Index of the first element in `paths` that is in the second path group.
///
/// In some cases there are two groups of paths. For instance, a renamed file can be accessed
/// through multiple source or destination paths (because of hard or symbolic links). Elements
/// in the `paths` vector located before this index are part of the first group (and thus refer
/// to the source file in the case of a rename), elements at or after the index are in the
/// secoud group (refering to the destination file).
#[cfg_attr(
feature = "serde",
serde(default, skip_serializing_if = "Option::is_none")
)]
path_group_split_index: Option<usize>,

/// The process ID of the originator of the event.
///
/// This attribute is experimental and, while included in Notify itself, is not considered
Expand Down Expand Up @@ -467,6 +485,10 @@ impl EventAttributes {
self.inner_mut().process_id = Some(process_id)
}

pub fn set_path_group_split_index(&mut self, path_group_split_index: usize) {
self.inner_mut().path_group_split_index = Some(path_group_split_index);
}

fn inner_mut(&mut self) -> &mut EventAttributesInner {
self.inner.get_or_insert_with(Box::default)
}
Expand Down Expand Up @@ -539,10 +561,16 @@ impl Event {
self
}

/// Adds a path to the event if the argument is Some.
pub fn add_some_path(self, path: Option<PathBuf>) -> Self {
if let Some(path) = path {
self.add_path(path)
/// Adds paths to the event.
pub fn add_paths(mut self, paths: &[PathBuf]) -> Self {
self.paths.extend_from_slice(paths);
self
}

/// Adds paths to the event if the argument is Some.
pub fn add_some_paths(self, paths: Option<&Vec<PathBuf>>) -> Self {
if let Some(paths) = paths {
self.add_paths(paths)
} else {
self
}
Expand Down Expand Up @@ -571,6 +599,13 @@ impl Event {
self.attrs.set_process_id(process_id);
self
}

/// Sets the tracker.
pub fn set_path_group_split_index(mut self, path_group_split_index: usize) -> Self {
self.attrs
.set_path_group_split_index(path_group_split_index);
self
}
}

impl fmt::Debug for Event {
Expand Down
124 changes: 78 additions & 46 deletions notify/src/inotify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ struct EventLoop {
event_handler: Box<dyn EventHandler>,
/// PathBuf -> (WatchDescriptor, WatchMask, is_recursive, is_dir)
watches: HashMap<PathBuf, (WatchDescriptor, WatchMask, bool, bool)>,
paths: HashMap<WatchDescriptor, PathBuf>,
paths: HashMap<WatchDescriptor, Vec<PathBuf>>,
rename_event: Option<Event>,
}

Expand All @@ -58,17 +58,19 @@ enum EventLoopMsg {

#[inline]
fn add_watch_by_event(
path: &Option<PathBuf>,
paths: &Option<Vec<PathBuf>>,
event: &inotify_sys::Event<&OsStr>,
watches: &HashMap<PathBuf, (WatchDescriptor, WatchMask, bool, bool)>,
add_watches: &mut Vec<PathBuf>,
) {
if let Some(ref path) = *path {
if let Some(ref paths) = *paths {
if event.mask.contains(EventMask::ISDIR) {
if let Some(parent_path) = path.parent() {
if let Some(&(_, _, is_recursive, _)) = watches.get(parent_path) {
if is_recursive {
add_watches.push(path.to_owned());
for path in paths {
if let Some(parent_path) = path.parent() {
if let Some(&(_, _, is_recursive, _)) = watches.get(parent_path) {
if is_recursive {
add_watches.push(path.to_owned());
}
}
}
}
Expand All @@ -78,13 +80,15 @@ fn add_watch_by_event(

#[inline]
fn remove_watch_by_event(
path: &Option<PathBuf>,
paths: &Option<Vec<PathBuf>>,
watches: &HashMap<PathBuf, (WatchDescriptor, WatchMask, bool, bool)>,
remove_watches: &mut Vec<PathBuf>,
) {
if let Some(ref path) = *path {
if watches.contains_key(path) {
remove_watches.push(path.to_owned());
if let Some(ref paths) = *paths {
for path in paths {
if watches.contains_key(path) {
remove_watches.push(path.to_owned());
}
}
}
}
Expand Down Expand Up @@ -212,20 +216,22 @@ impl EventLoop {
self.event_handler.handle_event(ev);
}

let path = match event.name {
Some(name) => self.paths.get(&event.wd).map(|root| root.join(name)),
let paths = match event.name {
Some(name) => self.paths.get(&event.wd).map(|roots| {
roots.iter().map(|root| root.join(name)).collect()
}),
None => self.paths.get(&event.wd).cloned(),
};

let mut evs = Vec::new();

if event.mask.contains(EventMask::MOVED_FROM) {
remove_watch_by_event(&path, &self.watches, &mut remove_watches);
remove_watch_by_event(&paths, &self.watches, &mut remove_watches);

let event = Event::new(EventKind::Modify(ModifyKind::Name(
RenameMode::From,
)))
.add_some_path(path.clone())
.add_some_paths(paths.as_ref())
.set_tracker(event.cookie as usize);

self.rename_event = Some(event.clone());
Expand All @@ -235,7 +241,7 @@ impl EventLoop {
evs.push(
Event::new(EventKind::Modify(ModifyKind::Name(RenameMode::To)))
.set_tracker(event.cookie as usize)
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);

let trackers_match = self
Expand All @@ -253,18 +259,19 @@ impl EventLoop {
RenameMode::Both,
)))
.set_tracker(event.cookie as usize)
.add_some_path(rename_event.paths.first().cloned())
.add_some_path(path.clone()),
.add_paths(&rename_event.paths)
.set_path_group_split_index(rename_event.paths.len())
.add_some_paths(paths.as_ref()),
);
}
add_watch_by_event(&path, &event, &self.watches, &mut add_watches);
add_watch_by_event(&paths, &event, &self.watches, &mut add_watches);
}
if event.mask.contains(EventMask::MOVE_SELF) {
evs.push(
Event::new(EventKind::Modify(ModifyKind::Name(
RenameMode::From,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
// TODO stat the path and get to new path
// - emit To and Both events
Expand All @@ -279,9 +286,9 @@ impl EventLoop {
CreateKind::File
},
))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
add_watch_by_event(&path, &event, &self.watches, &mut add_watches);
add_watch_by_event(&paths, &event, &self.watches, &mut add_watches);
}
if event.mask.contains(EventMask::DELETE) {
evs.push(
Expand All @@ -292,12 +299,12 @@ impl EventLoop {
RemoveKind::File
},
))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
remove_watch_by_event(&path, &self.watches, &mut remove_watches);
remove_watch_by_event(&paths, &self.watches, &mut remove_watches);
}
if event.mask.contains(EventMask::DELETE_SELF) {
let remove_kind = match &path {
let remove_kind = match paths.as_ref().and_then(|ps| ps.first()) {
Some(watched_path) => {
let current_watch = self.watches.get(watched_path);
match current_watch {
Expand All @@ -315,48 +322,48 @@ impl EventLoop {
};
evs.push(
Event::new(EventKind::Remove(remove_kind))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
remove_watch_by_event(&path, &self.watches, &mut remove_watches);
remove_watch_by_event(&paths, &self.watches, &mut remove_watches);
}
if event.mask.contains(EventMask::MODIFY) {
evs.push(
Event::new(EventKind::Modify(ModifyKind::Data(
DataChange::Any,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
}
if event.mask.contains(EventMask::CLOSE_WRITE) {
evs.push(
Event::new(EventKind::Access(AccessKind::Close(
AccessMode::Write,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
}
if event.mask.contains(EventMask::CLOSE_NOWRITE) {
evs.push(
Event::new(EventKind::Access(AccessKind::Close(
AccessMode::Read,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
}
if event.mask.contains(EventMask::ATTRIB) {
evs.push(
Event::new(EventKind::Modify(ModifyKind::Metadata(
MetadataKind::Any,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
}
if event.mask.contains(EventMask::OPEN) {
evs.push(
Event::new(EventKind::Access(AccessKind::Open(
AccessMode::Any,
)))
.add_some_path(path.clone()),
.add_some_paths(paths.as_ref()),
);
}

Expand Down Expand Up @@ -451,7 +458,7 @@ impl EventLoop {
let is_dir = metadata(&path).map_err(Error::io)?.is_dir();
self.watches
.insert(path.clone(), (w.clone(), watchmask, is_recursive, is_dir));
self.paths.insert(w, path);
self.paths.entry(w).or_default().push(path);
Ok(())
}
}
Expand All @@ -468,19 +475,40 @@ impl EventLoop {
let mut inotify_watches = inotify.watches();
log::trace!("removing inotify watch: {}", path.display());

inotify_watches
.remove(w.clone())
.map_err(|e| Error::io(e).add_path(path.clone()))?;
self.paths.remove(&w);
let remove_watch = self.paths.get_mut(&w).map_or(true, |paths| {
paths.retain(|p| p != &path);
paths.is_empty()
});
if remove_watch {
inotify_watches
.remove(w.clone())
.map_err(|e| Error::io(e).add_path(path.clone()))?;
self.paths.remove(&w);
}

if is_recursive || remove_recursive {
let mut remove_list = Vec::new();
for (w, p) in &self.paths {
if p.starts_with(&path) {
inotify_watches
.remove(w.clone())
.map_err(|e| Error::io(e).add_path(p.into()))?;
self.watches.remove(p);
for (w, paths) in &mut self.paths {
let mut removed_path = None;
let remove_watch = {
paths.retain(|p| {
let remove = p.starts_with(&path);
if remove {
removed_path = self.watches.remove_entry(p).map(|(k, _)| k);
}
!remove
});
paths.is_empty()
};

if remove_watch {
inotify_watches.remove(w.clone()).map_err(|e| {
let mut err = Error::io(e);
if let Some(p) = removed_path {
err = err.add_path(p);
}
err
})?;
remove_list.push(w.clone());
}
}
Expand All @@ -498,9 +526,13 @@ impl EventLoop {
if let Some(ref mut inotify) = self.inotify {
let mut inotify_watches = inotify.watches();
for (w, p) in &self.paths {
inotify_watches
.remove(w.clone())
.map_err(|e| Error::io(e).add_path(p.into()))?;
inotify_watches.remove(w.clone()).map_err(|e| {
let mut err = Error::io(e);
if let Some(path) = p.first() {
err = err.add_path(path.into());
}
err
})?;
}
self.watches.clear();
self.paths.clear();
Expand Down