diff --git a/src/events.rs b/src/events.rs index a3e2fc7..f5f5994 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1,13 +1,11 @@ use std::{ ffi::{OsStr, OsString}, mem, - os::unix::ffi::OsStrExt, - sync::Weak, + os::{fd::BorrowedFd, unix::ffi::OsStrExt}, }; use inotify_sys as ffi; -use crate::fd_guard::FdGuard; use crate::watches::WatchDescriptor; /// Iterator over inotify events @@ -18,15 +16,15 @@ use crate::watches::WatchDescriptor; /// [`Inotify::read_events_blocking`]: crate::Inotify::read_events_blocking /// [`Inotify::read_events`]: crate::Inotify::read_events #[derive(Debug)] -pub struct Events<'a> { - fd: Weak, +pub struct Events<'a, 'i> { + fd: BorrowedFd<'i>, buffer: &'a [u8], num_bytes: usize, pos: usize, } -impl<'a> Events<'a> { - pub(crate) fn new(fd: Weak, buffer: &'a [u8], num_bytes: usize) -> Self { +impl<'a, 'i> Events<'a, 'i> { + pub(crate) fn new(fd: BorrowedFd<'i>, buffer: &'a [u8], num_bytes: usize) -> Self { Events { fd, buffer, @@ -36,8 +34,8 @@ impl<'a> Events<'a> { } } -impl<'a> Iterator for Events<'a> { - type Item = Event<&'a OsStr>; +impl<'a, 'i> Iterator for Events<'a, 'i> { + type Item = Event<'i, &'a OsStr>; fn next(&mut self) -> Option { if self.pos < self.num_bytes { @@ -62,7 +60,7 @@ impl<'a> Iterator for Events<'a> { /// [`Inotify::read_events_blocking`]: crate::Inotify::read_events_blocking /// [`Inotify::read_events`]: crate::Inotify::read_events #[derive(Clone, Debug)] -pub struct Event { +pub struct Event<'i, S> { /// Identifies the watch this event originates from /// /// This [`WatchDescriptor`] is equal to the one that [`Watches::add`] @@ -73,7 +71,7 @@ pub struct Event { /// /// [`Watches::add`]: crate::Watches::add /// [`Watches::remove`]: crate::Watches::remove - pub wd: WatchDescriptor, + pub wd: WatchDescriptor<'i>, /// Indicates what kind of event this is pub mask: EventMask, @@ -96,8 +94,8 @@ pub struct Event { pub name: Option, } -impl<'a> Event<&'a OsStr> { - fn new(fd: Weak, event: &ffi::inotify_event, name: &'a OsStr) -> Self { +impl<'a, 'i> Event<'i, &'a OsStr> { + fn new(fd: BorrowedFd<'i>, event: &ffi::inotify_event, name: &'a OsStr) -> Self { let mask = EventMask::from_bits(event.mask) .expect("Failed to convert event mask. This indicates a bug."); @@ -123,7 +121,7 @@ impl<'a> Event<&'a OsStr> { /// # Panics /// /// Panics if the buffer does not contain a full event, including its name. - pub(crate) fn from_buffer(fd: Weak, buffer: &'a [u8]) -> (usize, Self) { + pub(crate) fn from_buffer(fd: BorrowedFd<'i>, buffer: &'a [u8]) -> (usize, Self) { let event_size = mem::size_of::(); // Make sure that the buffer is big enough to contain an event, without @@ -177,7 +175,7 @@ impl<'a> Event<&'a OsStr> { /// Returns an owned copy of the event. #[must_use = "cloning is often expensive and is not expected to have side effects"] - pub fn to_owned(&self) -> EventOwned { + pub fn to_owned(&self) -> Event<'i, OsString> { Event { wd: self.wd.clone(), mask: self.mask, @@ -188,7 +186,7 @@ impl<'a> Event<&'a OsStr> { } /// An owned version of `Event` -pub type EventOwned = Event; +pub type EventOwned<'i> = Event<'i, OsString>; bitflags! { /// Indicates the type of an event @@ -340,7 +338,7 @@ impl EventMask { #[cfg(test)] mod tests { - use std::{io::prelude::*, mem, slice, sync}; + use std::{fs::File, io::prelude::*, mem, os::fd::AsFd as _, slice}; use inotify_sys as ffi; @@ -367,9 +365,11 @@ mod tests { // After that event, simulate an event that starts with a non-zero byte. buffer[mem::size_of_val(event)] = 1; + let file = File::open("/dev/null").unwrap(); + let fd = file.as_fd(); // Now create the event and verify that the name is actually `None`, as // dictated by the value `len` above. - let (_, event) = Event::from_buffer(sync::Weak::new(), &buffer); + let (_, event) = Event::from_buffer(fd, &buffer); assert_eq!(event.name, None); } } diff --git a/src/fd_guard.rs b/src/fd_guard.rs deleted file mode 100644 index ceeaad6..0000000 --- a/src/fd_guard.rs +++ /dev/null @@ -1,83 +0,0 @@ -use std::{ - ops::Deref, - os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, RawFd}, - sync::atomic::{AtomicBool, Ordering}, -}; - -use inotify_sys as ffi; - -/// A RAII guard around a `RawFd` that closes it automatically on drop. -#[derive(Debug)] -pub struct FdGuard { - pub(crate) fd: RawFd, - pub(crate) close_on_drop: AtomicBool, -} - -impl FdGuard { - /// Indicate that the wrapped file descriptor should _not_ be closed - /// when the guard is dropped. - /// - /// This should be called in cases where ownership of the wrapped file - /// descriptor has been "moved" out of the guard. - /// - /// This is factored out into a separate function to ensure that it's - /// always used consistently. - #[inline] - pub fn should_not_close(&self) { - self.close_on_drop.store(false, Ordering::Release); - } -} - -impl Deref for FdGuard { - type Target = RawFd; - - #[inline] - fn deref(&self) -> &Self::Target { - &self.fd - } -} - -impl Drop for FdGuard { - fn drop(&mut self) { - if self.close_on_drop.load(Ordering::Acquire) { - unsafe { - ffi::close(self.fd); - } - } - } -} - -impl FromRawFd for FdGuard { - unsafe fn from_raw_fd(fd: RawFd) -> Self { - FdGuard { - fd, - close_on_drop: AtomicBool::new(true), - } - } -} - -impl IntoRawFd for FdGuard { - fn into_raw_fd(self) -> RawFd { - self.should_not_close(); - self.fd - } -} - -impl AsRawFd for FdGuard { - fn as_raw_fd(&self) -> RawFd { - self.fd - } -} - -impl AsFd for FdGuard { - #[inline] - fn as_fd(&self) -> BorrowedFd<'_> { - unsafe { BorrowedFd::borrow_raw(self.fd) } - } -} - -impl PartialEq for FdGuard { - fn eq(&self, other: &FdGuard) -> bool { - self.fd == other.fd - } -} diff --git a/src/inotify.rs b/src/inotify.rs index 640ae44..f12feab 100644 --- a/src/inotify.rs +++ b/src/inotify.rs @@ -2,14 +2,12 @@ use std::{ io, os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd}, path::Path, - sync::{atomic::AtomicBool, Arc}, }; use inotify_sys as ffi; use libc::{fcntl, F_GETFL, F_SETFL, O_NONBLOCK}; use crate::events::Events; -use crate::fd_guard::FdGuard; use crate::util::read_into_buffer; use crate::watches::{WatchDescriptor, WatchMask, Watches}; @@ -28,7 +26,7 @@ use crate::stream::EventStream; /// [top-level documentation]: crate #[derive(Debug)] pub struct Inotify { - fd: Arc, + fd: OwnedFd, } impl Inotify { @@ -85,18 +83,16 @@ impl Inotify { return Err(io::Error::last_os_error()); } - Ok(Inotify { - fd: Arc::new(FdGuard { - fd, - close_on_drop: AtomicBool::new(true), - }), - }) + // SAFETY: The resource pointed to by fd is open and suitable for assuming ownership. + let fd = unsafe { OwnedFd::from_raw_fd(fd) }; + + Ok(Inotify { fd }) } /// Gets an interface that allows adding and removing watches. /// See [`Watches::add`] and [`Watches::remove`]. pub fn watches(&self) -> Watches { - Watches::new(self.fd.clone()) + Watches::new(self.fd.as_fd()) } /// Deprecated: use `Inotify.watches().add()` instead @@ -122,23 +118,27 @@ impl Inotify { /// This method calls [`Inotify::read_events`] internally and behaves /// essentially the same, apart from the blocking behavior. Please refer to /// the documentation of [`Inotify::read_events`] for more information. - pub fn read_events_blocking<'a>(&mut self, buffer: &'a mut [u8]) -> io::Result> { + pub fn read_events_blocking<'a, 'i>( + &'i mut self, + buffer: &'a mut [u8], + ) -> io::Result> { unsafe { - let res = fcntl(**self.fd, F_GETFL); + let res = fcntl(self.fd.as_raw_fd(), F_GETFL); if res == -1 { return Err(io::Error::last_os_error()); } - if fcntl(**self.fd, F_SETFL, res & !O_NONBLOCK) == -1 { + if fcntl(self.fd.as_raw_fd(), F_SETFL, res & !O_NONBLOCK) == -1 { return Err(io::Error::last_os_error()); } }; + let raw_fd = self.fd.as_raw_fd(); let result = self.read_events(buffer); unsafe { - let res = fcntl(**self.fd, F_GETFL); + let res = fcntl(raw_fd, F_GETFL); if res == -1 { return Err(io::Error::last_os_error()); } - if fcntl(**self.fd, F_SETFL, res | O_NONBLOCK) == -1 { + if fcntl(raw_fd, F_SETFL, res | O_NONBLOCK) == -1 { return Err(io::Error::last_os_error()); } }; @@ -197,8 +197,8 @@ impl Inotify { /// [`read`]: libc::read /// [`ErrorKind::UnexpectedEof`]: std::io::ErrorKind::UnexpectedEof /// [`ErrorKind::InvalidInput`]: std::io::ErrorKind::InvalidInput - pub fn read_events<'a>(&mut self, buffer: &'a mut [u8]) -> io::Result> { - let num_bytes = read_into_buffer(**self.fd, buffer); + pub fn read_events<'a, 'i>(&'i mut self, buffer: &'a mut [u8]) -> io::Result> { + let num_bytes = read_into_buffer(self.fd.as_raw_fd(), buffer); let num_bytes = match num_bytes { 0 => { @@ -236,19 +236,7 @@ impl Inotify { } }; - Ok(Events::new(Arc::downgrade(&self.fd), buffer, num_bytes)) - } - - /// Deprecated: use `into_event_stream()` instead, which enforces a single `Stream` and predictable reads. - /// Using this method to create multiple `EventStream` instances from one `Inotify` is unsupported, - /// as they will contend over one event source and each produce unpredictable stream contents. - #[deprecated = "use `into_event_stream()` instead, which enforces a single Stream and predictable reads"] - #[cfg(feature = "stream")] - pub fn event_stream(&mut self, buffer: T) -> io::Result> - where - T: AsMut<[u8]> + AsRef<[u8]>, - { - EventStream::new(self.fd.clone(), buffer) + Ok(Events::new(self.fd.as_fd(), buffer, num_bytes)) } /// Create a stream which collects events. Consumes the `Inotify` instance. @@ -269,46 +257,9 @@ impl Inotify { /// initialized in `Inotify::init`. This is intended to be used to transform an /// `EventStream` back into an `Inotify`. Do not attempt to clone `Inotify` with this. #[cfg(feature = "stream")] - pub(crate) fn from_file_descriptor(fd: Arc) -> Self { + pub(crate) fn from_file_descriptor(fd: OwnedFd) -> Self { Inotify { fd } } - - /// Closes the inotify instance - /// - /// Closes the file descriptor referring to the inotify instance. The user - /// usually doesn't have to call this function, as the underlying inotify - /// instance is closed automatically, when [`Inotify`] is dropped. - /// - /// # Errors - /// - /// Directly returns the error from the call to [`close`], without adding any - /// error conditions of its own. - /// - /// # Examples - /// - /// ``` - /// use inotify::Inotify; - /// - /// let mut inotify = Inotify::init() - /// .expect("Failed to initialize an inotify instance"); - /// - /// inotify.close() - /// .expect("Failed to close inotify instance"); - /// ``` - /// - /// [`close`]: libc::close - pub fn close(self) -> io::Result<()> { - // `self` will be dropped when this method returns. If this is the only - // owner of `fd`, the `Arc` will also be dropped. The `Drop` - // implementation for `FdGuard` will attempt to close the file descriptor - // again, unless this flag here is cleared. - self.fd.should_not_close(); - - match unsafe { ffi::close(**self.fd) } { - 0 => Ok(()), - _ => Err(io::Error::last_os_error()), - } - } } impl AsRawFd for Inotify { @@ -321,7 +272,7 @@ impl AsRawFd for Inotify { impl FromRawFd for Inotify { unsafe fn from_raw_fd(fd: RawFd) -> Self { Inotify { - fd: Arc::new(FdGuard::from_raw_fd(fd)), + fd: OwnedFd::from_raw_fd(fd), } } } @@ -329,8 +280,7 @@ impl FromRawFd for Inotify { impl IntoRawFd for Inotify { #[inline] fn into_raw_fd(self) -> RawFd { - self.fd.should_not_close(); - self.fd.fd + self.fd.into_raw_fd() } } diff --git a/src/lib.rs b/src/lib.rs index 9546702..34f1fee 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -83,7 +83,6 @@ extern crate bitflags; mod events; -mod fd_guard; mod inotify; mod util; mod watches; diff --git a/src/stream.rs b/src/stream.rs index eead53e..cc969d0 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -1,8 +1,8 @@ use std::{ + ffi::OsStr, io, - os::unix::io::AsRawFd, + os::fd::{AsFd as _, AsRawFd as _, OwnedFd}, pin::Pin, - sync::Arc, task::{Context, Poll}, }; @@ -10,7 +10,6 @@ use futures_core::{ready, Stream}; use tokio::io::unix::AsyncFd; use crate::events::{Event, EventOwned}; -use crate::fd_guard::FdGuard; use crate::util::read_into_buffer; use crate::watches::Watches; use crate::Inotify; @@ -20,7 +19,7 @@ use crate::Inotify; /// Allows for streaming events returned by [`Inotify::into_event_stream`]. #[derive(Debug)] pub struct EventStream { - fd: AsyncFd>, + fd: AsyncFd, buffer: T, buffer_pos: usize, unused_bytes: usize, @@ -31,7 +30,7 @@ where T: AsMut<[u8]> + AsRef<[u8]>, { /// Returns a new `EventStream` associated with the default reactor. - pub(crate) fn new(fd: Arc, buffer: T) -> io::Result { + pub(crate) fn new(fd: OwnedFd, buffer: T) -> io::Result { Ok(EventStream { fd: AsyncFd::new(fd)?, buffer, @@ -43,7 +42,7 @@ where /// Returns an instance of `Watches` to add and remove watches. /// See [`Watches::add`] and [`Watches::remove`]. pub fn watches(&self) -> Watches { - Watches::new(self.fd.get_ref().clone()) + Watches::new(self.fd.as_fd()) } /// Consumes the `EventStream` instance and returns an `Inotify` using the original @@ -53,13 +52,13 @@ where } } -impl Stream for EventStream +impl<'i, T> Stream for EventStream where T: AsMut<[u8]> + AsRef<[u8]>, { - type Item = io::Result; + type Item = io::Result>; - fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + fn poll_next(self: Pin<&'i mut Self>, cx: &mut Context<'_>) -> Poll> { // Safety: safe because we never move out of `self_`. let self_ = unsafe { self.get_unchecked_mut() }; @@ -78,10 +77,8 @@ where // We have bytes in the buffer. inotify doesn't put partial events in // there, and we only take complete events out. That means we have at // least one event in there and can call `from_buffer` to take it out. - let (bytes_consumed, event) = Event::from_buffer( - Arc::downgrade(self_.fd.get_ref()), - &self_.buffer.as_ref()[self_.buffer_pos..], - ); + let (bytes_consumed, event): (usize, Event<&'i OsStr>) = + Event::from_buffer(self_.fd.as_fd(), &self_.buffer.as_ref()[self_.buffer_pos..]); self_.buffer_pos += bytes_consumed; self_.unused_bytes -= bytes_consumed; @@ -89,11 +86,7 @@ where } } -fn read( - fd: &AsyncFd>, - buffer: &mut [u8], - cx: &mut Context, -) -> Poll> { +fn read(fd: &AsyncFd, buffer: &mut [u8], cx: &mut Context) -> Poll> { let mut guard = ready!(fd.poll_read_ready(cx))?; let result = guard.try_io(|_| { let read = read_into_buffer(fd.as_raw_fd(), buffer); diff --git a/src/watches.rs b/src/watches.rs index 6f802cd..3de4693 100644 --- a/src/watches.rs +++ b/src/watches.rs @@ -3,16 +3,16 @@ use std::{ ffi::CString, hash::{Hash, Hasher}, io, - os::raw::c_int, - os::unix::ffi::OsStrExt, + os::{ + fd::{AsRawFd as _, BorrowedFd}, + raw::c_int, + unix::ffi::OsStrExt, + }, path::Path, - sync::{Arc, Weak}, }; use inotify_sys as ffi; -use crate::fd_guard::FdGuard; - bitflags! { /// Describes a file system watch /// @@ -230,7 +230,7 @@ impl WatchMask { } } -impl WatchDescriptor { +impl WatchDescriptor<'_> { /// Getter method for a watcher's id. /// /// Can be used to distinguish events for files with the same name. @@ -241,13 +241,13 @@ impl WatchDescriptor { /// Interface for adding and removing watches #[derive(Clone, Debug)] -pub struct Watches { - pub(crate) fd: Arc, +pub struct Watches<'i> { + pub(crate) fd: BorrowedFd<'i>, } -impl Watches { +impl<'i> Watches<'i> { /// Init watches with an inotify file descriptor - pub(crate) fn new(fd: Arc) -> Self { + pub(crate) fn new(fd: BorrowedFd<'i>) -> Self { Watches { fd } } @@ -314,20 +314,21 @@ impl Watches { /// ``` /// /// [`inotify_add_watch`]: inotify_sys::inotify_add_watch - pub fn add

(&mut self, path: P, mask: WatchMask) -> io::Result + pub fn add

(&mut self, path: P, mask: WatchMask) -> io::Result> where P: AsRef, { let path = CString::new(path.as_ref().as_os_str().as_bytes())?; - let wd = - unsafe { ffi::inotify_add_watch(**self.fd, path.as_ptr() as *const _, mask.bits()) }; + let wd = unsafe { + ffi::inotify_add_watch(self.fd.as_raw_fd(), path.as_ptr() as *const _, mask.bits()) + }; match wd { -1 => Err(io::Error::last_os_error()), _ => Ok(WatchDescriptor { id: wd, - fd: Arc::downgrade(&self.fd), + fd: self.fd.clone(), }), } } @@ -383,14 +384,14 @@ impl Watches { /// [`io::Error`]: std::io::Error /// [`ErrorKind`]: std::io::ErrorKind pub fn remove(&mut self, wd: WatchDescriptor) -> io::Result<()> { - if wd.fd.upgrade().as_ref() != Some(&self.fd) { + if wd.fd.as_raw_fd() == wd.fd.as_raw_fd() { return Err(io::Error::new( io::ErrorKind::InvalidInput, "Invalid WatchDescriptor", )); } - let result = unsafe { ffi::inotify_rm_watch(**self.fd, wd.id) }; + let result = unsafe { ffi::inotify_rm_watch(self.fd.as_raw_fd(), wd.id) }; match result { 0 => Ok(()), -1 => Err(io::Error::last_os_error()), @@ -407,35 +408,32 @@ impl Watches { /// /// [`Event`]: crate::Event #[derive(Clone, Debug)] -pub struct WatchDescriptor { +pub struct WatchDescriptor<'i> { pub(crate) id: c_int, - pub(crate) fd: Weak, + pub(crate) fd: BorrowedFd<'i>, } -impl Eq for WatchDescriptor {} +impl Eq for WatchDescriptor<'_> {} -impl PartialEq for WatchDescriptor { +impl PartialEq for WatchDescriptor<'_> { fn eq(&self, other: &Self) -> bool { - let self_fd = self.fd.upgrade(); - let other_fd = other.fd.upgrade(); - - self.id == other.id && self_fd.is_some() && self_fd == other_fd + self.id == other.id && self.fd.as_raw_fd() == other.fd.as_raw_fd() } } -impl Ord for WatchDescriptor { +impl Ord for WatchDescriptor<'_> { fn cmp(&self, other: &Self) -> Ordering { self.id.cmp(&other.id) } } -impl PartialOrd for WatchDescriptor { +impl PartialOrd for WatchDescriptor<'_> { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -impl Hash for WatchDescriptor { +impl Hash for WatchDescriptor<'_> { fn hash(&self, state: &mut H) { // This function only takes `self.id` into account, as `self.fd` is a // weak pointer that might no longer be available. Since neither diff --git a/tests/main.rs b/tests/main.rs index 2ad4486..145fe4d 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -6,7 +6,7 @@ use inotify::{Inotify, WatchMask}; use std::fs::File; use std::io::{ErrorKind, Write}; -use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd}; +use std::os::unix::io::{AsRawFd, IntoRawFd}; use std::path::PathBuf; use tempfile::TempDir; @@ -246,7 +246,7 @@ fn watch_descriptor_equality_should_not_be_confused_by_reused_fds() { let wd_1 = inotify_1.watches().add(&path, WatchMask::ACCESS).unwrap(); let fd_1 = inotify_1.as_raw_fd(); - inotify_1.close().unwrap(); + drop(inotify_1); let inotify_2 = Inotify::init().unwrap(); if fd_1 == inotify_2.as_raw_fd() { @@ -261,7 +261,7 @@ fn watch_descriptor_equality_should_not_be_confused_by_reused_fds() { // though, so they shouldn't be equal. assert!(wd_1 != wd_2); - inotify_2.close().unwrap(); + drop(inotify_2); // A little extra gotcha: If both inotify instances are closed, and the `Eq` // implementation naively compares the weak pointers, both will be `None`, @@ -278,14 +278,12 @@ fn it_should_implement_raw_fd_traits_correctly() { // If `IntoRawFd` has been implemented naively, `Inotify`'s `Drop` // implementation will have closed the inotify instance at this point. Let's // make sure this didn't happen. - let mut inotify = unsafe { ::from_raw_fd(fd) }; - let mut buffer = [0; 1024]; - if let Err(error) = inotify.read_events(&mut buffer) { - if error.kind() != ErrorKind::WouldBlock { - panic!("Failed to add watch: {}", error); - } - } + assert_eq!(unsafe { libc::fcntl(fd, libc::F_GETFD) }, -1); + assert_eq!( + std::io::Error::last_os_error().kind(), + std::io::Error::from_raw_os_error(libc::EBADF).kind() + ); } #[test]