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

Add "borrowed-fd" feature for sending BorrowedFd on a socket #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbranch
Copy link

@cbranch cbranch commented Jan 24, 2024

This is a more ergonomic way to use slices of ownership-safe file descriptors instead of coercing them into raw objects.

This is a more ergonomic way to use slices of ownership-safe file
descriptors instead of coercing them into raw objects.
@nagisa
Copy link
Collaborator

nagisa commented Jan 24, 2024

@Ekleog @lovesegfault I distinctly remember us considering something like this back when we were developing this crate and finding reasons to go with RawFds. Do either of you remember what it was about?

@nagisa
Copy link
Collaborator

nagisa commented Jan 24, 2024

Ah, never mind, these are io-safety types rather than conversion traits. Ignore my Q.

@nagisa
Copy link
Collaborator

nagisa commented Jan 24, 2024

seems good to me implementation wise though I can't say I love the duplication. My intuition is that we could make all our apis follow the i/o safety paradigm and just drop the RawFd apis entirely (or at least seriously consider making them take a less prominent spot.)

Unfortunately we cannot use AsFd or similar to support both options, because these APIs take slices of homogenous things...

@Ekleog
Copy link
Collaborator

Ekleog commented Jan 24, 2024

Unfortunately we cannot use AsFd or similar to support both options, because these APIs take slices of homogenous things...

Can we have an unsafe trait ReprRawFd trait that's implemented only for things that are #[repr(transparent)] RawFd? Currently that'd be just RawFd, BorrowedFd and OwnedFd.

Then we could take &[impl ReprRawFd] and transmute into the &[RawFd], thus keeping all the APIs as one, though the user would still have to make sure the inputs are homogeneous, potentially by using .as_fd() themselves.

@cbranch
Copy link
Author

cbranch commented Jan 30, 2024

Couldn't this just be an impl AsRawFd? While Rust has the I/O safe types, there isn't any clear or immediate plan to deprecate AsRawFd, and I don't think you make use of #[repr(transparent}] anywhere.
This is enough to get rid of all the duplication and support BorrowedFd; an unintended effect is you can now pass slices of File, UnixStream, Arc<UdpSocket> etc.

@nagisa
Copy link
Collaborator

nagisa commented Jan 31, 2024

Issue with either of these trait suggestions is that you cannot construct a [File, Socket, UnixStream]. Even if we accept that majority of the use-cases send a single fd (and so care not for homogenous lists), the underlying native functions require an [i32]. Allowing users to pass any impl AsRawFd would require allocating a vector internally to convert whatever has been passed in into a vector of i32 file descriptors. repr(transparent) would allow transmuting/reinterpreting instead allocation, which although quite yucky for other reasons, it at least avoids that allocation.

@cbranch
Copy link
Author

cbranch commented Jan 31, 2024

You can never construct [File, Socket, UnixStream] because it's impossible to unify them to a single type [T]. The closest you can get to a heterogenous list is to put them behind a &dyn Trait like so: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=70af83d5c7fd604146a1a51740753213
But this is not a common technique and if you really need heterogeneity, calling 'as_fd' on every input is just as convenient.

In any case the allocation argument is moot since you're writing directly into an allocated cmsghdr, there is no opportunity to reuse the input slice.

@nagisa
Copy link
Collaborator

nagisa commented Jan 31, 2024

calling 'as_fd' on every input is just as convenient.

I wouldn't be surprised if this was the reason we originally went with &[RawFd]s as an argument :)

I was thinking back to my message on whether we would be better off with replacing RawFd APIs right away, but I realized, that in we would want the recv side to move over to OwnedFds as well. Perhaps a &mut [Option<OwnedFd>] 🤔?

The current duplication until we find another good reason to make a breaking change might not be too terrible either, but you will need to adjust the CI before this can land regardless.

@Ekleog
Copy link
Collaborator

Ekleog commented Feb 2, 2024

Actually thinking about it some more, there is one way to do that:

pub trait AsFdSlice {
    fn write_fd_slice<'a>(&'a self, buf: &'a mut Vec<RawFd>) -> &'a [RawFd];
}

impl AsFdSlice for &[RawFd] {
    fn write_fd_slice<'a>(&'a self, _: &'a mut Vec<RawFd>) -> &'a [RawFd] {
        self
    }
}

impl<A: AsRawFd, B: AsRawFd> AsFdSlice for (A, B) {
    fn write_fd_slice<'a>(&'a self, buf: &'a mut Vec<RawFd>) -> &'a [RawFd] {
        buf.clear();
        buf.reserve_exact(2);
        buf.push(self.0.as_raw_fd());
        buf.push(self.1.as_raw_fd());
        &buf
    }
}

// etc., implemented by a macro for tuples up to 16 objects

Then we could take impl AsFdSlice as the parameter for sending, and just have a thread-local reusable buffer for the case where people want to send a (File, Socket) (because File is not repr(transparent) RawFd)

WDYT?

@nagisa
Copy link
Collaborator

nagisa commented Feb 2, 2024

Shrug. I know I kinda started the discussion here, but it feels like a waste of brainpower to give this crate too much design work and making it do something super fancy given that there's an unstable libstd equivalent. Once that one is done baking this crate will have served its purpose. Any braincells going into making sending of file descriptors around simple, convenient, fast, zero-cost, sound or anything else are perhaps better spent there.

But if we must do something fancy, then this crate could at least serve as a testing ground for the libstd APIs. For instance they appear to have trouble with OS support matrix, which I think we never too much trouble with.

Not that I expect any of us to have the necessary bandwidth or motivation for such work (especially given the presence of somebody else already working on pushing this through on the libstd side.)

@cbranch
Copy link
Author

cbranch commented Feb 5, 2024

I think my progress is going to be limited here:

1 - regarding fanciness/deduplication, I don't have enough context here to understand the goals behind the intermediate buffer or OS support matrix; but a fn send_with_asrawfd<T: AsRawFd>(&self, bytes: &[u8], fds: &[T]) -> io::Result<usize>; would work just as effectively with BorrowedFd and the existing functions can be trivially delegated to this one, to avoid duplication.
2 - regarding CI, you'd have to pin tokio to <=1.26 for it to build with rustc 1.53; fine if you check in (or construct) the Cargo.lock, inappropriate otherwise

Though libstd progress aside, feels like there's at least another 18 months' usage out of this crate especially if targeting older rust.

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.

3 participants