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

Add select::FdSet::fds() method #1207

Merged
merged 8 commits into from Apr 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -15,11 +15,14 @@ This project adheres to [Semantic Versioning](http://semver.org/).
identity for filesystem checks per-thread.
(#[1163](https://github.com/nix-rust/nix/pull/1163))
- Derived `Ord`, `PartialOrd` for `unistd::Pid` (#[1189](https://github.com/nix-rust/nix/pull/1189))
- Added `select::FdSet::fds` method to iterate over file descriptors in a set.
zombiezen marked this conversation as resolved.
Show resolved Hide resolved

### Changed
- Changed `fallocate` return type from `c_int` to `()` (#[1201](https://github.com/nix-rust/nix/pull/1201))
- Enabled `sys::ptrace::setregs` and `sys::ptrace::getregs` on x86_64-unknown-linux-musl target
(#[1198](https://github.com/nix-rust/nix/pull/1198))
- `select::FdSet::contains` and `select::FdSet::highest` now operate on `&FdSet`
instead of `&mut FdSet`.

### Fixed

Expand Down
108 changes: 84 additions & 24 deletions src/sys/select.rs
@@ -1,4 +1,6 @@
use std::iter::FusedIterator;
use std::mem;
use std::ops::Range;
use std::os::unix::io::RawFd;
use std::ptr::{null, null_mut};
use libc::{self, c_int};
Expand Down Expand Up @@ -30,8 +32,9 @@ impl FdSet {
unsafe { libc::FD_CLR(fd, &mut self.0) };
}

pub fn contains(&mut self, fd: RawFd) -> bool {
unsafe { libc::FD_ISSET(fd, &mut self.0) }
pub fn contains(&self, fd: RawFd) -> bool {
let mut copy = self.0;
Copy link
Member

Choose a reason for hiding this comment

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

libc::FD_ISSET should be taking a *const pointer rather than *mut. I just opened a PR to fix that.
rust-lang/libc#1725

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside, if we're worried about the cost of this copy, then we might want to consider not deriving Copy for FdSet.

Copy link
Member

Choose a reason for hiding this comment

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

You know, that's an excellent point. On my system, FdSet is 128 bytes big. Users probably shouldn't copy such a time unless they really need to. However removing Copy doesn't necessarily prevent the value from ever being memcpy'd, and the consensus is that it's best to impl Copy regardless of size. See https://www.reddit.com/r/rust/comments/2xxjda/when_should_my_type_be_copy/ and https://internals.rust-lang.org/t/pre-rfc-copy-like-trait-for-large-types/11852/14 for some discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I think that for now we should proceed without waiting for rust-lang/libc#1725 . But copying the entire structure on each loop of the iterator is woefully inefficient. I think you should retain the &mut reference instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This sadly means that the iterator cannot implement Clone, but I don't see a good way around that.

unsafe { libc::FD_ISSET(fd, &mut copy) }
}

pub fn clear(&mut self) {
Expand All @@ -58,15 +61,32 @@ impl FdSet {
/// ```
///
/// [`select`]: fn.select.html
pub fn highest(&mut self) -> Option<RawFd> {
for i in (0..FD_SETSIZE).rev() {
let i = i as RawFd;
if unsafe { libc::FD_ISSET(i, self as *mut _ as *mut libc::fd_set) } {
return Some(i)
}
}
pub fn highest(&self) -> Option<RawFd> {
self.fds().next_back()
}

None
/// Returns an iterator over the file descriptors in the set.
///
/// # Examples
///
/// ```
/// # extern crate nix;
/// # use nix::sys::select::FdSet;
/// # use std::os::unix::io::RawFd;
/// # fn main() {
zombiezen marked this conversation as resolved.
Show resolved Hide resolved
/// let mut set = FdSet::new();
/// set.insert(4);
/// set.insert(9);
/// let fds: Vec<RawFd> = set.fds().collect();
/// assert_eq!(fds, vec![4, 9]);
/// # }
/// ```
#[inline]
pub fn fds(&self) -> Fds {
Copy link
Member

Choose a reason for hiding this comment

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

Typically a C programmer will know what the maximum file descriptor he registered was, and when inspecting the fd_set he won't need to iterate all the way to FD_SETSIZE, which can be up to 64k on some platforms. But as written your iterator will always iterate that high. That's not good for performance. What if you add an max: Option<RawFd> argument tofds? If supplied, the iterator won't check any higher than that. Such an argument could also be useful to highest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to fds(). I didn't add to highest() in the interest of not introducing unnecessary breakage. If someone wants to get that performance gain, they can use set.fds(Some(my_limit)).next_back(), which isn't terribly hard to write and is pretty clear.

Fds {
set: self,
range: 0..FD_SETSIZE,
}
}
}

Expand All @@ -76,6 +96,46 @@ impl Default for FdSet {
}
}

/// Iterator over `FdSet`.
#[derive(Clone, Debug)]
pub struct Fds<'a> {
set: &'a FdSet,
range: Range<usize>,
}

impl<'a> Iterator for Fds<'a> {
type Item = RawFd;

fn next(&mut self) -> Option<RawFd> {
while let Some(i) = self.range.next() {
if self.set.contains(i as RawFd) {
return Some(i as RawFd);
}
}
None
}

#[inline]
fn size_hint(&self) -> (usize, Option<usize>) {
let (_, upper) = self.range.size_hint();
(0, upper)
}
}

impl<'a> DoubleEndedIterator for Fds<'a> {
#[inline]
fn next_back(&mut self) -> Option<RawFd> {
while let Some(i) = self.range.next_back() {
if self.set.contains(i as RawFd) {
return Some(i as RawFd);
}
}
None
}
}

impl<'a> FusedIterator for Fds<'a> {}

/// Monitors file descriptors for readiness
///
/// Returns the total number of ready file descriptors in all sets. The sets are changed so that all
Expand All @@ -100,9 +160,9 @@ impl Default for FdSet {
///
/// [`FdSet::highest`]: struct.FdSet.html#method.highest
pub fn select<'a, N, R, W, E, T>(nfds: N,
readfds: R,
writefds: W,
errorfds: E,
readfds: R,
writefds: W,
errorfds: E,
timeout: T) -> Result<c_int>
where
N: Into<Option<c_int>>,
Expand All @@ -129,7 +189,7 @@ where
let writefds = writefds.map(|set| set as *mut _ as *mut libc::fd_set).unwrap_or(null_mut());
let errorfds = errorfds.map(|set| set as *mut _ as *mut libc::fd_set).unwrap_or(null_mut());
let timeout = timeout.map(|tv| tv as *mut _ as *mut libc::timeval)
.unwrap_or(null_mut());
.unwrap_or(null_mut());

let res = unsafe {
libc::select(nfds, readfds, writefds, errorfds, timeout)
Expand Down Expand Up @@ -168,10 +228,10 @@ where
///
/// [`FdSet::highest`]: struct.FdSet.html#method.highest
pub fn pselect<'a, N, R, W, E, T, S>(nfds: N,
readfds: R,
writefds: W,
errorfds: E,
timeout: T,
readfds: R,
writefds: W,
errorfds: E,
timeout: T,
sigmask: S) -> Result<c_int>
where
N: Into<Option<c_int>>,
Expand Down Expand Up @@ -311,9 +371,9 @@ mod tests {

let mut timeout = TimeVal::seconds(10);
assert_eq!(1, select(Some(fd_set.highest().unwrap() + 1),
&mut fd_set,
None,
None,
&mut fd_set,
None,
None,
&mut timeout).unwrap());
assert!(fd_set.contains(r1));
assert!(!fd_set.contains(r2));
Expand All @@ -331,9 +391,9 @@ mod tests {

let mut timeout = TimeVal::seconds(10);
assert_eq!(1, select(::std::cmp::max(r1, r2) + 1,
&mut fd_set,
None,
None,
&mut fd_set,
None,
None,
&mut timeout).unwrap());
assert!(fd_set.contains(r1));
assert!(!fd_set.contains(r2));
Expand Down