diff --git a/src/events.rs b/src/events.rs index a3e2fc7..9cc3f64 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1,13 +1,12 @@ use std::{ ffi::{OsStr, OsString}, mem, - os::unix::ffi::OsStrExt, + os::{fd::OwnedFd, unix::ffi::OsStrExt}, sync::Weak, }; use inotify_sys as ffi; -use crate::fd_guard::FdGuard; use crate::watches::WatchDescriptor; /// Iterator over inotify events @@ -19,14 +18,14 @@ use crate::watches::WatchDescriptor; /// [`Inotify::read_events`]: crate::Inotify::read_events #[derive(Debug)] pub struct Events<'a> { - fd: Weak, + fd: Weak, 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 { + pub(crate) fn new(fd: Weak, buffer: &'a [u8], num_bytes: usize) -> Self { Events { fd, buffer, @@ -97,7 +96,7 @@ pub struct Event { } impl<'a> Event<&'a OsStr> { - fn new(fd: Weak, event: &ffi::inotify_event, name: &'a OsStr) -> Self { + fn new(fd: Weak, 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 +122,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: Weak, buffer: &'a [u8]) -> (usize, Self) { let event_size = mem::size_of::(); // Make sure that the buffer is big enough to contain an event, without 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..db5767f 100644 --- a/src/inotify.rs +++ b/src/inotify.rs @@ -2,14 +2,13 @@ use std::{ io, os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, OwnedFd, RawFd}, path::Path, - sync::{atomic::AtomicBool, Arc}, + sync::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 +27,7 @@ use crate::stream::EventStream; /// [top-level documentation]: crate #[derive(Debug)] pub struct Inotify { - fd: Arc, + fd: Arc, } impl Inotify { @@ -85,12 +84,10 @@ 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: Arc::new(fd) }) } /// Gets an interface that allows adding and removing watches. @@ -124,21 +121,21 @@ impl Inotify { /// the documentation of [`Inotify::read_events`] for more information. pub fn read_events_blocking<'a>(&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 result = self.read_events(buffer); 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()); } }; @@ -198,7 +195,7 @@ impl Inotify { /// [`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); + let num_bytes = read_into_buffer(self.fd.as_raw_fd(), buffer); let num_bytes = match num_bytes { 0 => { @@ -269,46 +266,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: Arc) -> 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 +281,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: Arc::new(OwnedFd::from_raw_fd(fd)), } } } @@ -329,8 +289,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.as_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..c50c521 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -1,6 +1,6 @@ use std::{ io, - os::unix::io::AsRawFd, + os::fd::{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: Arc, buffer: T) -> io::Result { Ok(EventStream { fd: AsyncFd::new(fd)?, buffer, @@ -90,7 +89,7 @@ where } fn read( - fd: &AsyncFd>, + fd: &AsyncFd>, buffer: &mut [u8], cx: &mut Context, ) -> Poll> { diff --git a/src/watches.rs b/src/watches.rs index 6f802cd..e3c9520 100644 --- a/src/watches.rs +++ b/src/watches.rs @@ -3,16 +3,17 @@ use std::{ ffi::CString, hash::{Hash, Hasher}, io, - os::raw::c_int, - os::unix::ffi::OsStrExt, + os::{ + fd::{AsRawFd as _, OwnedFd}, + 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 /// @@ -242,12 +243,12 @@ impl WatchDescriptor { /// Interface for adding and removing watches #[derive(Clone, Debug)] pub struct Watches { - pub(crate) fd: Arc, + pub(crate) fd: Arc, } impl Watches { /// Init watches with an inotify file descriptor - pub(crate) fn new(fd: Arc) -> Self { + pub(crate) fn new(fd: Arc) -> Self { Watches { fd } } @@ -320,8 +321,9 @@ impl Watches { { 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()), @@ -383,14 +385,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.upgrade().as_ref().map(|fd| fd.as_raw_fd()) != Some(self.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()), @@ -409,7 +411,7 @@ impl Watches { #[derive(Clone, Debug)] pub struct WatchDescriptor { pub(crate) id: c_int, - pub(crate) fd: Weak, + pub(crate) fd: Weak, } impl Eq for WatchDescriptor {} @@ -419,7 +421,11 @@ impl PartialEq for WatchDescriptor { 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 + && match (self_fd, other_fd) { + (Some(fd1), Some(fd2)) => fd1.as_raw_fd() == fd2.as_raw_fd(), + _ => false, + } } } 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]