kmc-solid: I/O safety #115159

kawadakk · 2023-08-24T05:09:01Z

Adds the I/O safety API (#87329) for socket file descriptors in *-kmc-solid_* Tier 3 targets. All new public items are gated by the solid_ext library feature.

This PR adds the following public types and traits:

std::os::solid::io::AsFd
std::os::solid::io::BorrowedFd
std::os::solid::io::OwnedFd

std::os::solid::prelude::AsFd (re-export)
std::os::solid::prelude::BorrowedFd (re-export)
std::os::solid::prelude::OwnedFd (re-export)

And trait implementations:

From<std::net::TcpListener> for std::os::solid::io::OwnedFd
From<std::net::TcpStream> for std::os::solid::io::OwnedFd
From<std::net::UdpSocket> for std::os::solid::io::OwnedFd
From<std::os::solid::io::OwnedFd> for std::net::TcpListener
From<std::os::solid::io::OwnedFd> for std::net::TcpStream
From<std::os::solid::io::OwnedFd> for std::net::UdpSocket
std::fmt::Debug for std::os::solid::io::BorrowedFd<'_>
std::fmt::Debug for std::os::solid::io::OwnedFd
std::io::IsTerminal for std::os::solid::io::BorrowedFd<'_>
std::io::IsTerminal for std::os::solid::io::OwnedFd
std::os::fd::AsRawFd for std::os::solid::io::BorrowedFd<'_>
std::os::fd::AsRawFd for std::os::solid::io::OwnedFd
std::os::fd::FromRawFd for std::os::solid::io::OwnedFd
std::os::fd::IntoRawFd for std::os::solid::io::OwnedFd
std::os::solid::io::AsFd for &impl std::os::solid::io::AsFd
std::os::solid::io::AsFd for &mut impl std::os::solid::io::AsFd
std::os::solid::io::AsFd for Arc<impl std::os::solid::io::AsFd>
std::os::solid::io::AsFd for Box<impl std::os::solid::io::AsFd>
std::os::solid::io::AsFd for Rc<impl std::os::solid::io::AsFd>
std::os::solid::io::AsFd for std::net::TcpListener
std::os::solid::io::AsFd for std::net::TcpStream
std::os::solid::io::AsFd for std::net::UdpSocket
std::os::solid::io::AsFd for std::os::solid::io::BorrowedFd<'_>
std::os::solid::io::AsFd for std::os::solid::io::OwnedFd

Taking advantage of the above change, this PR also refactors the internal details of std::sys::solid::net to match the design of other targets, e.g., by redefining Socket as a newtype of OwnedFd.

rustbot · 2023-08-24T05:09:08Z

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2023-08-26T23:48:57Z

r? libs-api -- not sure if we ask for ACPs for extending Tier-3 target APIs.

workingjubilee · 2023-10-11T20:07:38Z

It would be unprecedented to ask for such when it is merely extending a target-neutral std API for a tier 3 target.

workingjubilee · 2023-10-11T20:10:47Z

library/std/src/os/solid/io.rs

+#[derive(Copy, Clone)]
+#[repr(transparent)]
+#[rustc_layout_scalar_valid_range_start(0)]
+// This is -2, in two's complement. -1 is `SOLID_NET_INVALID_FD`.
+#[rustc_layout_scalar_valid_range_end(0xFF_FF_FF_FE)]
+#[rustc_nonnull_optimization_guaranteed]
+pub struct BorrowedFd<'socket> {
+    fd: RawFd,
+    _phantom: PhantomData<&'socket OwnedFd>,
+}


This layout optimization has come into question on Haiku:

Disable the BorrowedFd layout optimization on Haiku #112371

I would appreciate it if you weighed in with your thoughts, either here or in that PR.

This BorrowedFd is specific to the SOLID socket subsystem, so technically it's closer to std::os::windows::io::BorrowedSocket than std::os::fd::BorrowedFd, the POSIX file descriptor that is in question in the referred issue.

Personally, AT_FDCWD feels like a special value that can only be handled by a few supported functions. It's also unusual in that it doesn't point to one static entity over its lifetime, which I suspect may break assumption of some code. I'd be fine with fd::BorrowedFd only being able to hold a "valid" value and leaving the ability to hold such special values unspecified.

Even the POSIX specification's wordings suggest that it's not a file descriptor:

The <fcntl.h> header shall define the following symbolic constant [AT_FDCWD] as a special value used in place of a file descriptor [...]

oh, interesting! I hadn't realized that SOLID was that divergent!

bors · 2023-10-19T14:00:48Z

☔ The latest upstream changes (presumably #116132) made this pull request unmergeable. Please resolve the merge conflicts.

It's mostly based on `std::os::fd::owned`.

Mostly copied from `std::os::unix::io`, except quantifying file descriptors with SOLID Sockets and removing the paragraph mentioning `mmap`.

…pStream,TcpListener,UdpSocket}`

…`Socket` Follows how other targets are implemented.

Removes the private type `std::sys::solid::net::FileDesc`, replacing its only usage in `std::sys::solid::net::Socket` with `std::os::solid::io:: OwnedFd`.

…et}` by delegating to inner `Socket` Removes some `unsafe` blocks.

…ude`

kawadakk · 2023-11-08T04:23:39Z

Rebased on top of master.

workingjubilee · 2023-11-22T21:11:41Z

As it would be unprecedented to make std-approved API expansion for a tier 3 platform dependent on explicit T-libs-api approval, I am shipping this.
@bors r+

bors · 2023-11-22T21:11:44Z

📌 Commit 52eb92d has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2023-11-23T00:04:01Z

⌛ Testing commit 52eb92d with merge e450511...

…r=workingjubilee kmc-solid: I/O safety Adds the I/O safety API (rust-lang#87329) for socket file descriptors in [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets. All new public items are gated by the `solid_ext` library feature. This PR adds the following public types and traits: std::os::solid::io::AsFd std::os::solid::io::BorrowedFd std::os::solid::io::OwnedFd std::os::solid::prelude::AsFd (re-export) std::os::solid::prelude::BorrowedFd (re-export) std::os::solid::prelude::OwnedFd (re-export) And trait implementations: From<std::net::TcpListener> for std::os::solid::io::OwnedFd From<std::net::TcpStream> for std::os::solid::io::OwnedFd From<std::net::UdpSocket> for std::os::solid::io::OwnedFd From<std::os::solid::io::OwnedFd> for std::net::TcpListener From<std::os::solid::io::OwnedFd> for std::net::TcpStream From<std::os::solid::io::OwnedFd> for std::net::UdpSocket std::fmt::Debug for std::os::solid::io::BorrowedFd<'_> std::fmt::Debug for std::os::solid::io::OwnedFd std::io::IsTerminal for std::os::solid::io::BorrowedFd<'_> std::io::IsTerminal for std::os::solid::io::OwnedFd std::os::fd::AsRawFd for std::os::solid::io::BorrowedFd<'_> std::os::fd::AsRawFd for std::os::solid::io::OwnedFd std::os::fd::FromRawFd for std::os::solid::io::OwnedFd std::os::fd::IntoRawFd for std::os::solid::io::OwnedFd std::os::solid::io::AsFd for &impl std::os::solid::io::AsFd std::os::solid::io::AsFd for &mut impl std::os::solid::io::AsFd std::os::solid::io::AsFd for Arc<impl std::os::solid::io::AsFd> std::os::solid::io::AsFd for Box<impl std::os::solid::io::AsFd> std::os::solid::io::AsFd for Rc<impl std::os::solid::io::AsFd> std::os::solid::io::AsFd for std::net::TcpListener std::os::solid::io::AsFd for std::net::TcpStream std::os::solid::io::AsFd for std::net::UdpSocket std::os::solid::io::AsFd for std::os::solid::io::BorrowedFd<'_> std::os::solid::io::AsFd for std::os::solid::io::OwnedFd Taking advantage of the above change, this PR also refactors the internal details of `std::sys::solid::net` to match the design of other targets, e.g., by redefining `Socket` as a newtype of `OwnedFd`.

bors · 2023-11-23T00:29:06Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-11-23T00:29:19Z

The job dist-x86_64-netbsd failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

workingjubilee · 2023-11-23T03:54:47Z

@bors retry

bors · 2023-11-23T04:24:13Z

⌛ Testing commit 52eb92d with merge 1934665...

bors · 2023-11-23T06:21:23Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 1934665 to master...

rust-timer · 2023-11-23T09:11:03Z

Finished benchmarking commit (1934665): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.231s -> 675.802s (-0.21%)
Artifact size: 313.70 MiB -> 313.70 MiB (0.00%)

rustbot assigned Mark-Simulacrum Aug 24, 2023

rustbot added O-solid Operating System: SOLID S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 24, 2023

rustbot assigned BurntSushi and unassigned Mark-Simulacrum Aug 26, 2023

Mark-Simulacrum assigned m-ou-se and unassigned BurntSushi Aug 26, 2023

workingjubilee reviewed Oct 11, 2023

View reviewed changes

kawadakk added 9 commits November 8, 2023 10:48

kmc-solid: Add std::os::solid::io::{BorrowedFd,OwnedFd,AsFd}

5d3aefe

It's mostly based on `std::os::fd::owned`.

kmc-solid: Document I/O safety in std::os::solid::io

ddfe168

Mostly copied from `std::os::unix::io`, except quantifying file descriptors with SOLID Sockets and removing the paragraph mentioning `mmap`.

kmc-solid: Implement AsFd and conversion to/from OwnedFd for `{Tc…

cf9c4a3

…pStream,TcpListener,UdpSocket}`

kmc-solid: Implement AsFd for {Arc,Rc,Box}<impl AsFd>

0dd3b25

kmc-solid: Replace {From,Into}Inner<c_int> impls with *RawFd for …

cbfab81

…`Socket` Follows how other targets are implemented.

kmc-solid: Remove FileDesc

6d1e4dd

Removes the private type `std::sys::solid::net::FileDesc`, replacing its only usage in `std::sys::solid::net::Socket` with `std::os::solid::io:: OwnedFd`.

kmc-solid: Implement {From,Into}Inner<OwnedFd> for Socket

46bc247

kmc-solid: Reimplement AsFd etc for `{TcpStream,TcpListener,UdpSock…

c8c3339

…et}` by delegating to inner `Socket` Removes some `unsafe` blocks.

kmc-solid: Re-export {As,Borrowed,Owned}Fd in `std::os::solid::prel…

52eb92d

…ude`

kawadakk force-pushed the patch/kmc-solid/io-safety branch from 3d66f9b to 52eb92d Compare November 8, 2023 04:17

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 23, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2023

bors merged commit 1934665 into rust-lang:master Nov 23, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 23, 2023

kawadakk deleted the patch/kmc-solid/io-safety branch November 24, 2023 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kmc-solid: I/O safety #115159

kmc-solid: I/O safety #115159

kawadakk commented Aug 24, 2023 •

edited

rustbot commented Aug 24, 2023

Mark-Simulacrum commented Aug 26, 2023

workingjubilee commented Oct 11, 2023

workingjubilee Oct 11, 2023 •

edited

kawadakk Oct 13, 2023

workingjubilee Oct 13, 2023 •

edited

bors commented Oct 19, 2023

kawadakk commented Nov 8, 2023

workingjubilee commented Nov 22, 2023 •

edited

bors commented Nov 22, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-log-analyzer commented Nov 23, 2023

workingjubilee commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

kmc-solid: I/O safety #115159

kmc-solid: I/O safety #115159

Conversation

kawadakk commented Aug 24, 2023 • edited

rustbot commented Aug 24, 2023

Mark-Simulacrum commented Aug 26, 2023

workingjubilee commented Oct 11, 2023

workingjubilee Oct 11, 2023 • edited

Choose a reason for hiding this comment

kawadakk Oct 13, 2023

Choose a reason for hiding this comment

workingjubilee Oct 13, 2023 • edited

Choose a reason for hiding this comment

bors commented Oct 19, 2023

kawadakk commented Nov 8, 2023

workingjubilee commented Nov 22, 2023 • edited

bors commented Nov 22, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-log-analyzer commented Nov 23, 2023

workingjubilee commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

kawadakk commented Aug 24, 2023 •

edited

workingjubilee Oct 11, 2023 •

edited

workingjubilee Oct 13, 2023 •

edited

workingjubilee commented Nov 22, 2023 •

edited