From 8ce03c6475495ae725dd88497823e6c0c1dbf890 Mon Sep 17 00:00:00 2001 From: Hugo Osvaldo Barrera Date: Tue, 14 Jan 2025 11:51:42 +0100 Subject: [PATCH] Replace FdGuard with OwnedFd An OwnedFd provides the same guarantees: that the underlying file descriptor is closed only once. The file descriptor is behind an Arc, so it will be closed once the last copy of the Arc is dropped. Events and Watches now have a lifetime referencing the original file descriptor, since they have a BorrowedFd referencing it. --- src/events.rs | 36 +++++++++---------- src/fd_guard.rs | 83 -------------------------------------------- src/inotify.rs | 92 +++++++++++-------------------------------------- src/lib.rs | 1 - src/stream.rs | 29 ++++++---------- src/watches.rs | 52 ++++++++++++++-------------- tests/main.rs | 18 +++++----- 7 files changed, 83 insertions(+), 228 deletions(-) delete mode 100644 src/fd_guard.rs 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]