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

feat: I/O safety for 'sys/signal' #1936

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Dec 10, 2022

What does this PR do

  1. Adds I/O safety for sys/signal.

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

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Apart from the doc nit, this LGTM. However, it may cause borrow checker issues upstack. The change works for me in mio-aio, but it could cause difficulty if Tokio adopts I/O Safety. I think we should probably merge it, but work with Tokio to ensure that all of the pieces will fit together before we release.

src/sys/signal.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member Author

macOS CI failure: network issue

    Updating crates.io index
warning: spurious network error (2 tries remaining): SecureTransport error: (null); class=Net (12)
warning: spurious network error (1 tries remaining): SecureTransport error: (null); class=Net (12)
error: failed to get `bitflags` as a dependency of package `nix v0.26.1 (/private/var/folders/hy/gdmd645x1ws410vx266t7py00000gn/T/cirrus-ci-build)`

Caused by:
  failed to load source for dependency `bitflags`

Caused by:
  Unable to update registry `crates-io`

Caused by:
  failed to fetch `https://github.com/rust-lang/crates.io-index`

Caused by:
  network failure seems to have happened
  if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  SecureTransport error: (null); class=Net (12)

Exit status: 101

@SteveLauC
Copy link
Member Author

but it could cause difficulty if Tokio adopts I/O Safety.

Hi @asomers, I think we can merge this now, looks like Tokio won't adopt I/O safety.

@SteveLauC SteveLauC mentioned this pull request Jun 9, 2024
29 tasks
/// variants.
#[doc(hidden)]
#[cfg(not(freebsdlike))]
_Unreachable(&'fd std::convert::Infallible),
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the associated type of this variant to &'fd Infallible, since Infallible is an enum with 0 variants, this variant can now never be constructed. (As a contrast, PhantomData is very easy to create)

@@ -1,6 +1,3 @@
// Portions of this file are Copyright 2014 The Rust Project Developers.
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this header, looks like it is the only header we have.

@SteveLauC SteveLauC added this to the 0.30.0 milestone Jun 9, 2024
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

I just checked again. There's no way to make this work with Tokio except by using BorrowedFd::borrow_raw, which defeats the whole point of I/O safety. But we can merge it if there are other users who can benefit.

@SteveLauC
Copy link
Member Author

There's no way to make this work with Tokio except by using BorrowedFd::borrow_raw

Ah, right, since Tokio does not adopt I/O safety (in its implementation), all its internal interfaces are using RawFd, so it has to do the tedious conversion when interacting with I/O-safe interfaces.

@SteveLauC SteveLauC added this pull request to the merge queue Jun 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 10, 2024
@SteveLauC SteveLauC added this pull request to the merge queue Jun 10, 2024
Merged via the queue into nix-rust:master with commit 51e552a Jun 10, 2024
36 checks passed
@SteveLauC SteveLauC deleted the io-safety-signal branch June 10, 2024 02:55
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.

2 participants