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

Replace FdGuard with OwnedFd #233

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

Replace FdGuard with OwnedFd #233

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
11 changes: 5 additions & 6 deletions src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -19,14 +18,14 @@ use crate::watches::WatchDescriptor;
/// [`Inotify::read_events`]: crate::Inotify::read_events
#[derive(Debug)]
pub struct Events<'a> {
fd: Weak<FdGuard>,
fd: Weak<OwnedFd>,
buffer: &'a [u8],
num_bytes: usize,
pos: usize,
}

impl<'a> Events<'a> {
pub(crate) fn new(fd: Weak<FdGuard>, buffer: &'a [u8], num_bytes: usize) -> Self {
pub(crate) fn new(fd: Weak<OwnedFd>, buffer: &'a [u8], num_bytes: usize) -> Self {
Events {
fd,
buffer,
Expand Down Expand Up @@ -97,7 +96,7 @@ pub struct Event<S> {
}

impl<'a> Event<&'a OsStr> {
fn new(fd: Weak<FdGuard>, event: &ffi::inotify_event, name: &'a OsStr) -> Self {
fn new(fd: Weak<OwnedFd>, 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.");

Expand All @@ -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<FdGuard>, buffer: &'a [u8]) -> (usize, Self) {
pub(crate) fn from_buffer(fd: Weak<OwnedFd>, buffer: &'a [u8]) -> (usize, Self) {
let event_size = mem::size_of::<ffi::inotify_event>();

// Make sure that the buffer is big enough to contain an event, without
Expand Down
83 changes: 0 additions & 83 deletions src/fd_guard.rs
View file
Open in desktop

This file was deleted.

69 changes: 14 additions & 55 deletions src/inotify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -28,7 +27,7 @@ use crate::stream::EventStream;
/// [top-level documentation]: crate
#[derive(Debug)]
pub struct Inotify {
fd: Arc<FdGuard>,
fd: Arc<OwnedFd>,
}

impl Inotify {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<Events<'a>> {
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());
}
};
Expand Down Expand Up @@ -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<Events<'a>> {
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 => {
Expand Down Expand Up @@ -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<FdGuard>) -> Self {
pub(crate) fn from_file_descriptor(fd: Arc<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()),
}
}
Comment on lines -276 to -311
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather keep this method as a no-op. The documentation should change accordingly.

I'd be fine with deprecating it, as it's superfluous. But I'd like to keep the breaking changes to a minimum

}

impl AsRawFd for Inotify {
Expand All @@ -321,16 +281,15 @@ 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)),
}
}
}

impl IntoRawFd for Inotify {
#[inline]
fn into_raw_fd(self) -> RawFd {
self.fd.should_not_close();
self.fd.fd
self.fd.as_raw_fd()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be problematic; the RawFd can outlive the OwnedFd past the point where it has been closed.

We can't convert the OwnedFd into a RawFd, because it's behind an Arc, so we don't actually own it here.

In practice, actually converting the RawFd into anything is unsafe, so we're not breaking any rules or leaving room for any undefined behaviour.

This is somewhat of an API change, given that the previous implementation leaked the file descriptor instead, and some consumers MAY rely on this (although technically, that would be unsound and depending on an undocumented implementation detail).

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this can actually work. Most likely, self.fd is the only copy of the Arc, which means OwnedFd will be dropped right here, and the file descriptor will be closed. This would render this whole IntoRawFd implementation non-functional.

I think the only way this could work, is if the Arc has been cloned. Which, if I'm not missing anything, only happens if somebody called watches and is keeping that Watches instance around indefinitely.

This ties into the discussion in #227. And I think it's a sign that the current weird way of doing it is just wrong 😄

I think we should change Watches to have an fd: &OwnedFd instead (which would require a lifetime on the whole struct). Then we can get rid of the Arc and call into_raw_fd here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would render this whole IntoRawFd implementation non-functional.

This returns a RawFd. Converting that into anything "usable" (via FromRawFd) is always unsafe, because the consumer has no way of ensuring that the Fd hasn't been closed.

I think that eventually, Inotify should implement std::os::fd::AsFd, which uses lifetimes to ensure that the fd is not closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's try to address #227 first anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

The lack of guarantees by RawFd is a meaningless technicality in this context.

IntoRawFd has a specific purpose, to transfer ownership of the file descriptor. Right now, it fulfills that purpose. With the change proposed here, it would close the file descriptor on transfer (unless the user is doing something weird and specific with another part of the API), rendering the transfer meaningless. And most likely breaking any working applications that currently use this API as intended.

In that case, the IntoRawFd implementation might as well not exist. In fact, that would be the preferable option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was thinking of AsRawFd, but this is IntoRawFd.

}
}

Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@
extern crate bitflags;

mod events;
mod fd_guard;
mod inotify;
mod util;
mod watches;
Expand Down
9 changes: 4 additions & 5 deletions src/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{
io,
os::unix::io::AsRawFd,
os::fd::{AsRawFd as _, OwnedFd},
pin::Pin,
sync::Arc,
task::{Context, Poll},
Expand All @@ -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;
Expand All @@ -20,7 +19,7 @@ use crate::Inotify;
/// Allows for streaming events returned by [`Inotify::into_event_stream`].
#[derive(Debug)]
pub struct EventStream<T> {
fd: AsyncFd<Arc<FdGuard>>,
fd: AsyncFd<Arc<OwnedFd>>,
buffer: T,
buffer_pos: usize,
unused_bytes: usize,
Expand All @@ -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<FdGuard>, buffer: T) -> io::Result<Self> {
pub(crate) fn new(fd: Arc<OwnedFd>, buffer: T) -> io::Result<Self> {
Ok(EventStream {
fd: AsyncFd::new(fd)?,
buffer,
Expand Down Expand Up @@ -90,7 +89,7 @@ where
}

fn read(
fd: &AsyncFd<Arc<FdGuard>>,
fd: &AsyncFd<Arc<OwnedFd>>,
buffer: &mut [u8],
cx: &mut Context,
) -> Poll<io::Result<usize>> {
Expand Down
Loading
Loading