Add a socketstream_seqpacket feature.

[sunfishcode]

Add a socketstream_seqpacket feature, which uses SOCK_SEQPACKET on
Unix-type platforms and PIPE_TYPE_MESSAGE on Windows.

[cgwalters]

Wow, that was fast! 🚀

OK I'll try this out soon.

[cgwalters]

I haven't seen this style before of "early Ok wrapping then map()". What's the advantage over something like the following which I feel like I've seen much more commonly in other Rust code?

diff --git a/src/unix.rs b/src/unix.rs
index 9f8c876..ea58b29 100644
--- a/src/unix.rs
+++ b/src/unix.rs
@@ -55,22 +55,18 @@ pub fn socketpair_stream() -> io::Result<(SocketpairStream, SocketpairStream)> {
 /// Create a socketpair and return seqpacket handles connected to each end.
 #[inline]
 pub fn socketpair_seqpacket() -> io::Result<(SocketpairStream, SocketpairStream)> {
-    Ok(rustix::net::socketpair(
+    let (a, b) = rustix::net::socketpair(
         AddressFamily::UNIX,
         SocketType::SEQPACKET,
         SocketFlags::CLOEXEC,
         Protocol::default(),
-    )
-    .map(|(a, b)| {
-        let a = a.into_raw_fd();
-        let b = b.into_raw_fd();
-        unsafe {
-            (
-                SocketpairStream::from_raw_fd(a),
-                SocketpairStream::from_raw_fd(b),
-            )
-        }
-    })?)
+    )?;
+    unsafe {
+        Ok((
+            SocketpairStream::from_raw_fd(a.into_raw_fd()),
+            SocketpairStream::from_raw_fd(b.into_raw_fd()),
+        ))
+    }
 }
 
 impl Read for SocketpairStream {

[sunfishcode]

No particular advantage; it just fell out from how the code evolved one step at a time. I've changed it to your suggestion now, which I agree is easier to read :-).

[cgwalters]

(My goal here is to drop the unsafe code from https://github.com/containers/containers-image-proxy-rs to start, and ideally also switch over to using tokio instead of blocking writes)

Here's what I have so far that compiles:

diff --git a/Cargo.toml b/Cargo.toml
index b7b584c..2e01248 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,6 +18,11 @@ serde_json = "1.0.64"
 semver = "1.0.4"
 tokio = { features = ["full"], version = "1" }
 tracing = "0.1"
+socketpair = { version = "0.10.2-alpha.0" }
 
 [lib]
 path = "src/imageproxy.rs"
+
+
+[patch.crates-io]
+socketpair = { path = "../../sunfishcode/socketpair" }
diff --git a/src/imageproxy.rs b/src/imageproxy.rs
index c9d9718..cb67f7c 100644
--- a/src/imageproxy.rs
+++ b/src/imageproxy.rs
@@ -10,8 +10,9 @@ use nix::sys::socket::{self as nixsocket, ControlMessageOwned};
 use nix::sys::uio::IoVec;
 use serde::{Deserialize, Serialize};
 use std::fs::File;
+use std::io::Write;
 use std::os::unix::io::AsRawFd;
-use std::os::unix::prelude::{FromRawFd, RawFd};
+use std::os::unix::prelude::{FromRawFd, IntoRawFd, RawFd};
 use std::pin::Pin;
 use std::process::Stdio;
 use std::sync::{Arc, Mutex};
@@ -70,7 +71,7 @@ type ChildFuture = Pin<Box<dyn Future<Output = std::io::Result<std::process::Out
 
 /// Manage a child process proxy to fetch container images.
 pub struct ImageProxy {
-    sockfd: Arc<Mutex<File>>,
+    sockfd: Arc<Mutex<socketpair::SocketpairStream>>,
     childwait: ChildFuture,
 }
 
@@ -84,20 +85,6 @@ impl std::fmt::Debug for ImageProxy {
 #[derive(Debug, PartialEq, Eq)]
 pub struct OpenedImage(u32);
 
-#[allow(unsafe_code)]
-fn new_seqpacket_pair() -> Result<(File, File)> {
-    let (mysock, theirsock) = nixsocket::socketpair(
-        nixsocket::AddressFamily::Unix,
-        nixsocket::SockType::SeqPacket,
-        None,
-        nixsocket::SockFlag::SOCK_CLOEXEC,
-    )?;
-    // Convert to owned values
-    let mysock = unsafe { std::fs::File::from_raw_fd(mysock) };
-    let theirsock = unsafe { std::fs::File::from_raw_fd(theirsock) };
-    Ok((mysock, theirsock))
-}
-
 #[allow(unsafe_code)]
 fn file_from_scm_rights(cmsg: ControlMessageOwned) -> Option<File> {
     if let nixsocket::ControlMessageOwned::ScmRights(fds) = cmsg {
@@ -124,7 +111,8 @@ impl ImageProxy {
     /// Create an image proxy that fetches the target image
     #[instrument]
     pub async fn new_with_config(config: ImageProxyConfig) -> Result<Self> {
-        let (mysock, theirsock) = new_seqpacket_pair()?;
+        let (mysock, theirsock) = socketpair::socketpair_seqpacket()?;
+        let theirsock = unsafe { std::fs::File::from_raw_fd(theirsock.into_raw_fd()) };
         // By default, we use util-linux's `setpriv` to set up pdeathsig to "lifecycle bind"
         // the child process to us.  In the future we should allow easily configuring
         // e.g. systemd-run as a wrapper, etc.
@@ -162,15 +150,15 @@ impl ImageProxy {
     }
 
     async fn impl_request_raw<T: serde::de::DeserializeOwned + Send + 'static>(
-        sockfd: Arc<Mutex<File>>,
+        sockfd: Arc<Mutex<socketpair::SocketpairStream>>,
         req: Request,
     ) -> Result<(T, Option<(File, u32)>)> {
         tracing::trace!("sending request {}", req.method.as_str());
         // TODO: Investigate https://crates.io/crates/uds for SOCK_SEQPACKET tokio
         let r = tokio::task::spawn_blocking(move || {
-            let sockfd = sockfd.lock().unwrap();
+            let mut sockfd = sockfd.lock().unwrap();
             let sendbuf = serde_json::to_vec(&req)?;
-            nixsocket::send(sockfd.as_raw_fd(), &sendbuf, nixsocket::MsgFlags::empty())?;
+            sockfd.write_all(&sendbuf)?;
             drop(sendbuf);
             let mut buf = [0u8; MAX_MSG_SIZE];
             let mut cmsg_buffer = nix::cmsg_space!([RawFd; 1]);

Two questions:

• Is there a crate somewhere in the rustix ecosystem that allows fd passing to child processes without that unsafe conversion to File I have? I guess this would be impl Into<Stdio> for OwnedFd` in io-lifetimes maybe?
• Is there an API to do the SCM_RIGHTS stuff to replace the nix bits?

[sunfishcode]

For eliminating that unsafe conversion, that's io-lifetimes. SocketpairStream here implements io-lifetimes' IntoFd trait, which has an into_fd() which returns an OwnedFd, and then you can do File::from_fd on that to get a File, courtesy of the FromFd trait.

SCM_RIGHTS unfortunately isn't implemented in the rustix ecosystem yet. There's nothing blocking it, it just takes care to design and implement it right.

Though if you need Windows support, that's more complex, as Windows doesn't support any form of SCM_RIGHTS. One process with access to the process handle for another can put handles in the other via DuplicateHandle, but that has a pretty different shape.

[cgwalters]

For eliminating that unsafe conversion, that's io-lifetimes. SocketpairStream here implements io-lifetimes' IntoFd trait, which has an into_fd() which returns an OwnedFd, and then you can do File::from_fd on that to get a File, courtesy of the FromFd trait.

👍

Though if you need Windows support, that's more complex, as Windows doesn't support any form of SCM_RIGHTS.

No need for Windows for me.

SCM_RIGHTS unfortunately isn't implemented in the rustix ecosystem yet. There's nothing blocking it, it just takes care to design and implement it right.

Yeah, that's what I was suspecting. Makes this more of a mixed bag for me since I'd be using rustix and nix. Hmm.

That said...while this is obviously up to you, I'd still say this is worth merging. I might try to do a push to do rustix porting anyways in our ecosystem.

[sunfishcode]

Yeah, that's what I was suspecting. Makes this more of a mixed bag for me since I'd be using rustix and nix. Hmm.

Yeah, though I expect we will eventually implement SCM_RIGHTS as I'm expecting other use cases for it as well.

That said...while this is obviously up to you, I'd still say this is worth merging. I might try to do a push to do rustix porting anyways in our ecosystem.

Very cool! I'd be interested to hear how it goes :-).