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

Replace direct dependency on libc with rustix #31

Merged
merged 3 commits into from
Dec 30, 2022
Merged

Conversation

taiki-e
Copy link
Collaborator

@taiki-e taiki-e commented Dec 30, 2022

@taiki-e taiki-e requested a review from notgull December 30, 2022 02:55
```
warning: the borrowed expression implements the required traits
   --> tests/std.rs:311:29
    |
311 |             cmd.env("PATH", &p);
    |                             ^^ help: change this to: `p`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `#[warn(clippy::needless_borrow)]` on by default
```
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Looks good to me!

src/lib.rs Outdated
syscall!(fcntl(fd, libc::F_SETFL, res & !libc::O_NONBLOCK));

fn blocking_fd(fd: rustix::fd::BorrowedFd<'_>) -> io::Result<()> {
let flags = rustix::fs::fcntl_getfl(fd)?;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at libstd, on Linux it uses FIONBIO instead of this strategy. We might want to try that instead.

Copy link
Collaborator Author

@taiki-e taiki-e Dec 30, 2022

Choose a reason for hiding this comment

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

That behavior in the standard library seems to have been introduced in rust-lang/rust@efeb42b.

I wondered why it is linux-only, since FIONBIO is available on several other unix-like systems. After a little looking into it, in the standard library it seems mainly because it could not find docs on other operating systems (rust-lang/rust#39514 (comment)), and also this comment in libuv indicates that there are cases where it is not supported, at least on BSD. Also, this commit in mio indicates that it is not working as we expected on illumos.

So, for now, it seems to make sense to do it only on linux.

@taiki-e taiki-e merged commit 7980b46 into master Dec 30, 2022
@taiki-e taiki-e deleted the taiki-e/rustix branch December 30, 2022 04:43
@notgull notgull mentioned this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants