-
Notifications
You must be signed in to change notification settings - Fork 654
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
Implement I/O Safety #1750
Comments
I lack the knowledge of how you'd approach this, but the main takeaway from the
Once bumping MSRV becomes acceptable for the project, you can drop |
Given that Rust 1.63.0 just dropped and I/O safety is now in the standard library, it might be worth it to bump the MSRV. If the bump is desired, I can implement the change, moving the input file descriptors to |
I've got a wip branch changing around half so far of the uses of RawFd to the io-safe versions. |
@coolreader18 Mind trying to merge what you've got? I don't see the need to convert everything at once... any progress in this direction is a net improvement IMO. |
@asomers I'd be willing to take a crack at this over the weekend assuming there's review bandwidth. |
I've been thinking about this some more... what's our backwards compat story? Do we add an io_safety feature that has the new wrappers and deprecate all the old ones? Or do we just pull the plug and make everyone use the new types? cc @rtzoeller |
Since we're pre-1.0, we don't technically have to maintain backwards-compat. And in the past, we sometimes haven't. If you were only going to modify one or two functions, I'd say add new methods and deprecate the old ones. But this change is way too big. Better to pull the plug all at once, I'd say. |
As it's partially a safety concern, I think a break would be justified, as significant as it would be. |
Sounds good! Based on discussion over at rustix, I'll need to update my PR to not take fds in by reference. #1862 needs to go through first anyway, so I'll update stuff after that. Another possibility that's crossed my mind: we could have an extension trait on fds so that you can say |
The extension trait sounds confusing to me, just because it's such a big break from Nix's existing API. |
#1874 adds I/O safety to signalfd |
It might be worth annotating what are covered in #1863 to avoid duplicate work:
BTW, they are modules |
@asomers @SteveLauC for APIs like pty, I think we should replace |
Also worth noting that the convention I'm going with is to take fds by value ( |
For the first reason why we should take
In Nix, we are doing something like: fn path_related_fn<P: NixPath>(path: &P) |
I vote for pass-by-value, unless somebody can come up with a single case where it wouldn't work. After all, the standard library already provides |
I just updated this tracking issue, looking at those unfinished modules, most of them are in PR #1863, friendly ping @SUPERCILEX, would you like to split the parts that won't involve the design issue (non-xxat functions) to other PRs |
@SteveLauC I won't have time to work on this, so I would recommend taking my first commit from that PR (30b32c4) and resolving the conflicts. |
Most Nix functions that involve files use
RawFd
arguments, but those are difficult to use correctly. For example, it's easy to use-after-close. Rust nightly just added I/O Safety traits that fix most of the problems withRawFd
. It's a very good fit for Nix. We should begin using them as soon as possible. They're on track to be released as part of Rust 1.63.0, which will probably be released on 11-August.https://github.com/rust-lang/rfcs/blob/master/text/3128-io-safety.md
The text was updated successfully, but these errors were encountered: