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

FdSet: allow conversion from iterable of AsFd #2383

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rhelmot
Copy link

@rhelmot rhelmot commented Apr 22, 2024

What does this PR do

Adds an implementation of FromIterator and From for FdSet, allowing them to be constructed from iterables of types that implement AsFd.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@TheJonny
Copy link
Contributor

TheJonny commented Apr 23, 2024

I'm not sure what the better type is, but I think the iterator item type should be the same as the argument type ofFdSet::insert, so that the implementation would be just

for fd in iter.into_iter() {
    result.insert(fd)
}

instead of duplicating the unsafe implementation of insert.

would that be ok: 7def327 ?

Or would it be an API breaking change to also change insert?

@rhelmot
Copy link
Author

rhelmot commented Apr 23, 2024

The goal here was definitely to avoid having to add a map in the user's construction code. What if we factored the unsafe code into another function that both insert and this could use?

@TheJonny
Copy link
Contributor

TheJonny commented Apr 23, 2024

src/poll.rs has a comment why BorrowedFd is used as argument type instead f F: AsFd, which also applies here, I think:

// Unlike I/O functions, constructors like this must take `BorrowedFd`

AsFd would allow passing an OwnedFd or File, which would be closed on drop, leaving the FdSet with closed fds.

you can reproduce it if you change your test a bit

fn test_fdset_from_iterable() {
    let (r1, w1) = pipe().unwrap();
    let (r2, w2) = pipe().unwrap();
    let reads = [r1, r2]
        .into_iter()
        .map(|fd| (fd.as_raw_fd(), fd))
        .collect::<std::collections::HashMap<_, _>>();
    let writes = [w1, w2];
    let reads_fdset: FdSet = reads.values().into();
    let writes_fdset: FdSet = writes.into();
    for w in writes_fdset.fds(None) {
        nix::unistd::write(w, b"x").expect("should work");
    }
   /// ...
}

panics with EBADF.

@SteveLauC
Copy link
Member

The FdSet interfaces indeed have a reason why they are using BorrowedFd rather than Fd: AsFd, see this PR


And for that unsafe code snippet,

unsafe { libc::FD_SET(fd.as_raw_fd(), &mut result.set) };

I am not sure I understand what this means,

The goal here was definitely to avoid having to add a map in the user's construction code.

would you like to elaborate on it a bit?

@TheJonny
Copy link
Contributor

The FdSet interfaces indeed have a reason

we might document it :)

@TheJonny
Copy link
Contributor

appart from type issue, I like the idea of the PR: having a way to construct an FdSet from an iterable!

@SteveLauC
Copy link
Member

The FdSet interfaces indeed have a reason

we might document it :)

Yeah, I think we can document it in the comments

@rhelmot
Copy link
Author

rhelmot commented Apr 28, 2024

would you like to elaborate on it a bit?

The goal was to avoid this snippet from the linked commit: writes.iter().map(|x| x.as_fd()).into()

I have not forgotten this PR, I am simply sidetracked onto other projects for a bit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants