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

Unify inotify public interface across backends and accept impl AsFd #1105

Merged
merged 2 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 0 additions & 53 deletions src/backend/libc/fs/inotify.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
//! inotify support for working with inotifies

use crate::backend::c;
use crate::backend::conv::{borrowed_fd, c_str, ret, ret_c_int, ret_owned_fd};
use crate::fd::{BorrowedFd, OwnedFd};
use crate::io;
use bitflags::bitflags;

bitflags! {
Expand Down Expand Up @@ -79,53 +76,3 @@ bitflags! {
const _ = !0;
}
}

/// `inotify_init1(flags)`—Creates a new inotify object.
///
/// Use the [`CreateFlags::CLOEXEC`] flag to prevent the resulting file
/// descriptor from being implicitly passed across `exec` boundaries.
#[doc(alias = "inotify_init1")]
pub fn inotify_init(flags: CreateFlags) -> io::Result<OwnedFd> {
// SAFETY: `inotify_init1` has no safety preconditions.
Copy link
Member

Choose a reason for hiding this comment

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

Could you restore these three SAFETY comments? I know they're not super interesting, but they're there for the benefit of people auditing the code. They record what we believed the relevant safety considerations are, so that if it ever turns out that we missed something, it's easier to understand the intent of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, restored.

unsafe { ret_owned_fd(c::inotify_init1(bitflags_bits!(flags))) }
}

/// `inotify_add_watch(self, path, flags)`—Adds a watch to inotify.
///
/// This registers or updates a watch for the filesystem path `path` and
/// returns a watch descriptor corresponding to this watch.
///
/// Note: Due to the existence of hardlinks, providing two different paths to
/// this method may result in it returning the same watch descriptor. An
/// application should keep track of this externally to avoid logic errors.
pub fn inotify_add_watch<P: crate::path::Arg>(
inot: BorrowedFd<'_>,
path: P,
flags: WatchFlags,
) -> io::Result<i32> {
path.into_with_c_str(|path| {
// SAFETY: The fd and path we are passing is guaranteed valid by the
// type system.
unsafe {
ret_c_int(c::inotify_add_watch(
borrowed_fd(inot),
c_str(path),
flags.bits(),
))
}
})
}

/// `inotify_rm_watch(self, wd)`—Removes a watch from this inotify.
///
/// The watch descriptor provided should have previously been returned by
/// [`inotify_add_watch`] and not previously have been removed.
#[doc(alias = "inotify_rm_watch")]
pub fn inotify_remove_watch(inot: BorrowedFd<'_>, wd: i32) -> io::Result<()> {
// Android's `inotify_rm_watch` takes `u32` despite that
// `inotify_add_watch` expects a `i32`.
#[cfg(target_os = "android")]
let wd = wd as u32;
// SAFETY: The fd is valid and closing an arbitrary wd is valid.
unsafe { ret(c::inotify_rm_watch(borrowed_fd(inot), wd)) }
}
36 changes: 36 additions & 0 deletions src/backend/libc/fs/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2549,3 +2549,39 @@ fn test_sizes() {
#[cfg(fix_y2038)]
assert!(core::mem::size_of::<[c::timespec; 2]>() < core::mem::size_of::<Timestamps>());
}

#[inline]
#[cfg(linux_kernel)]
pub(crate) fn inotify_init1(flags: super::inotify::CreateFlags) -> io::Result<OwnedFd> {
// SAFETY: `inotify_init1` has no safety preconditions.
unsafe { ret_owned_fd(c::inotify_init1(bitflags_bits!(flags))) }
}

#[inline]
#[cfg(linux_kernel)]
pub(crate) fn inotify_add_watch(
inot: BorrowedFd<'_>,
path: &CStr,
flags: super::inotify::WatchFlags,
) -> io::Result<i32> {
// SAFETY: The fd and path we are passing is guaranteed valid by the
// type system.
unsafe {
ret_c_int(c::inotify_add_watch(
borrowed_fd(inot),
c_str(path),
flags.bits(),
))
}
}

#[inline]
#[cfg(linux_kernel)]
pub(crate) fn inotify_rm_watch(inot: BorrowedFd<'_>, wd: i32) -> io::Result<()> {
// Android's `inotify_rm_watch` takes `u32` despite that
// `inotify_add_watch` expects a `i32`.
#[cfg(target_os = "android")]
let wd = wd as u32;
// SAFETY: The fd is valid and closing an arbitrary wd is valid.
unsafe { ret(c::inotify_rm_watch(borrowed_fd(inot), wd)) }
}
40 changes: 0 additions & 40 deletions src/backend/linux_raw/fs/inotify.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
//! inotify support for working with inotifies

use crate::backend::c;
use crate::backend::fs::syscalls;
use crate::fd::{BorrowedFd, OwnedFd};
use crate::io;
use bitflags::bitflags;

bitflags! {
Expand Down Expand Up @@ -79,40 +76,3 @@ bitflags! {
const _ = !0;
}
}

/// `inotify_init1(flags)`—Creates a new inotify object.
///
/// Use the [`CreateFlags::CLOEXEC`] flag to prevent the resulting file
/// descriptor from being implicitly passed across `exec` boundaries.
#[doc(alias = "inotify_init1")]
#[inline]
pub fn inotify_init(flags: CreateFlags) -> io::Result<OwnedFd> {
syscalls::inotify_init1(flags)
}

/// `inotify_add_watch(self, path, flags)`—Adds a watch to inotify.
///
/// This registers or updates a watch for the filesystem path `path` and
/// returns a watch descriptor corresponding to this watch.
///
/// Note: Due to the existence of hardlinks, providing two different paths to
/// this method may result in it returning the same watch descriptor. An
/// application should keep track of this externally to avoid logic errors.
#[inline]
pub fn inotify_add_watch<P: crate::path::Arg>(
inot: BorrowedFd<'_>,
path: P,
flags: WatchFlags,
) -> io::Result<i32> {
path.into_with_c_str(|path| syscalls::inotify_add_watch(inot, path, flags))
}

/// `inotify_rm_watch(self, wd)`—Removes a watch from this inotify.
///
/// The watch descriptor provided should have previously been returned by
/// [`inotify_add_watch`] and not previously have been removed.
#[doc(alias = "inotify_rm_watch")]
#[inline]
pub fn inotify_remove_watch(inot: BorrowedFd<'_>, wd: i32) -> io::Result<()> {
syscalls::inotify_rm_watch(inot, wd)
}
43 changes: 43 additions & 0 deletions src/fs/inotify.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//! inotify support for working with inotifies

pub use crate::backend::fs::inotify::{CreateFlags, WatchFlags};
use crate::backend::fs::syscalls;
use crate::fd::{AsFd, OwnedFd};
use crate::io;

/// `inotify_init1(flags)`—Creates a new inotify object.
///
/// Use the [`CreateFlags::CLOEXEC`] flag to prevent the resulting file
/// descriptor from being implicitly passed across `exec` boundaries.
#[doc(alias = "inotify_init1")]
#[inline]
pub fn inotify_init(flags: CreateFlags) -> io::Result<OwnedFd> {
syscalls::inotify_init1(flags)
}

/// `inotify_add_watch(self, path, flags)`—Adds a watch to inotify.
///
/// This registers or updates a watch for the filesystem path `path` and
/// returns a watch descriptor corresponding to this watch.
///
/// Note: Due to the existence of hardlinks, providing two different paths to
/// this method may result in it returning the same watch descriptor. An
/// application should keep track of this externally to avoid logic errors.
#[inline]
pub fn inotify_add_watch<P: crate::path::Arg>(
inot: impl AsFd,
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the TODO comments about changing this impl AsFd to a generic argument in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #753 (comment). I feel like it'll be more effective to run a global pass on the whole lib rather than trying to remember the spots impls need to be fixed. If you disagree, I'll be happy to add the TODOs here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense.

path: P,
flags: WatchFlags,
) -> io::Result<i32> {
path.into_with_c_str(|path| syscalls::inotify_add_watch(inot.as_fd(), path, flags))
}

/// `inotify_rm_watch(self, wd)`—Removes a watch from this inotify.
///
/// The watch descriptor provided should have previously been returned by
/// [`inotify_add_watch`] and not previously have been removed.
#[doc(alias = "inotify_rm_watch")]
#[inline]
pub fn inotify_remove_watch(inot: impl AsFd, wd: i32) -> io::Result<()> {
syscalls::inotify_rm_watch(inot.as_fd(), wd)
}
4 changes: 2 additions & 2 deletions src/fs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ mod getpath;
#[cfg(not(target_os = "wasi"))] // WASI doesn't have get[gpu]id.
mod id;
#[cfg(linux_kernel)]
pub mod inotify;
#[cfg(linux_kernel)]
mod ioctl;
#[cfg(not(any(
target_os = "espidf",
Expand Down Expand Up @@ -66,8 +68,6 @@ mod sync;
#[cfg(any(apple, linux_kernel, target_os = "hurd"))]
mod xattr;

#[cfg(linux_kernel)]
pub use crate::backend::fs::inotify;
pub use abs::*;
#[cfg(not(target_os = "redox"))]
pub use at::*;
Expand Down
Loading