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.

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

feat: I/O safety for 'sys/inotify' #1913

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Dec 5, 2022

What this PR does:

  1. Changes the fd field of struct Inotify from RawFd to OwnedFd

  2. Changes the interfaces of functions in the impl Inotify {}

    The type of self changes from Self to &mut Self.

    From:

    pub fn add_watch<P: ?Sized + NixPath>(
          self,
          path: &P,
          mask: AddWatchFlags,
    ) -> Result<WatchDescriptor> 
    
    pub fn rm_watch(self, wd: WatchDescriptor) -> Result<()>
    
    pub fn read_events(self) -> Result<Vec<InotifyEvent>>

    To:

    pub fn add_watch<P: ?Sized + NixPath>(
         &mut self,
         path: &P,
         mask: AddWatchFlags,
     ) -> Result<WatchDescriptor>
    
    pub fn rm_watch(&mut self, wd: WatchDescriptor) -> Result<()>
    
    pub fn read_events(&mut self) -> Result<Vec<InotifyEvent>>

    In the previous implementation, these functions can take self by value as struct Inotify was Copy. With the changes in 1 applied, struct Inotify is no longer Copy, so we have to take self by reference.


Blocks until the merge of #1863 as this PR needs read(2) to be I/O-safe.

@SteveLauC SteveLauC mentioned this pull request Dec 5, 2022
29 tasks
@asomers
Copy link
Member

asomers commented Dec 5, 2022

The CI failure looks directly related to this PR.

@SteveLauC
Copy link
Member Author

SteveLauC commented Dec 6, 2022

The CI failure looks directly related to this PR.

Yes, that's why this PR needs to be blocked until the merge of #1863

error[E0308]: mismatched types
    --> src/sys/inotify.rs:198:26
     |
198  |         let nread = read(&self.fd.as_fd(), &mut buffer)?;
     |                     ---- ^^^^^^^^^^^^^^^^ expected `i32`, found `&BorrowedFd<'_>`
     |                     |
     |                     arguments to this function are incorrect
     |
note: function defined here
    --> src/unistd.rs:1089:8
     |
1089 | pub fn read(fd: RawFd, buf: &mut [u8]) -> Result<usize> {
     |        ^^^^ ---------  --------------

@SteveLauC SteveLauC force-pushed the io-safety-sys/inotify branch 2 times, most recently from 457ec50 to 835ff2c Compare December 7, 2022 09:46
@SteveLauC
Copy link
Member Author

@asomers I have fixed the error (by using the old RawFd read(2) interface) and this is ready for review.

@@ -152,12 +152,12 @@ impl Inotify {
///
/// For more information see, [inotify_add_watch(2)](https://man7.org/linux/man-pages/man2/inotify_add_watch.2.html).
pub fn add_watch<P: ?Sized + NixPath>(
self,
&mut self,
Copy link
Member

Choose a reason for hiding this comment

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

Why &mut self and not &self?

Copy link
Member Author

Choose a reason for hiding this comment

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

Semantically, inotify_add_watch modifies the existing inotify instance, though this can not be reflected in the implementation of this binding.

Man 2 inotify_add_watch

Copy link
Member

Choose a reason for hiding this comment

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

I consider this is a case of "internal mutability". The correctness of the the method doesn't depend on it having exclusive access to the the structure, so we can remove the mut.

Copy link
Member Author

Choose a reason for hiding this comment

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

Get it, this can also be applied to rm_watch and read_events

@SteveLauC SteveLauC force-pushed the io-safety-sys/inotify branch from 835ff2c to 9d74330 Compare December 9, 2022 03:56
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit 3d3e6b9 into nix-rust:master Dec 9, 2022
@SteveLauC SteveLauC deleted the io-safety-sys/inotify branch December 10, 2022 00:49
@VorpalBlade
Copy link
Contributor

It seems this broke monitoring Inotify with Epoll since it removed the AsRawFd but didn't implement any other way to combine it will Epoll (or plain Poll for that matter). This makes it impossible to monitor both inotify files and some other FDs at the same time.

See also issue #1998

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants