unix: Extend UnixStream and UnixDatagram to send and receive file descriptors #69864
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I think these functions all need to take Other than that, the final code after your series of commits great to me. However, please don't submit a patch series which introduces and subsequently fixes bugs within the same series. Your patch series should consist of a set of commits that each individually pass tests. In this case, I think the patches can safely be squashed into one. Could you do that and force-push the result to the PR branch? With those issues fixed, r=me. |
|
Also, would you consider adding a |
|
Thank you for your feedback. The functions I do not recommend to make Should I squash the commits first and then make the changes (add |
|
On Thu, Mar 12, 2020 at 03:27:49AM -0700, LinkTed wrote:
The functions `send_to`, `send`, `recv_from` and `recv` of [`UnixDatagram`](https://doc.rust-lang.org/std/os/unix/net/struct.UnixDatagram.html) takes `&self`, should I change it to `&mut self` anyway?
Interesting! No, I stand corrected, leave them as `&self`. Sorry for the
incorrect suggestion.
I do not recommend to make `send_to` and `send_vectored_fds` to share a common implementation, because `send_to` use a different system call.
I would suggest that `send_vectored_to` and `send_vectored_fds_to`
should share a common implementation, at least, since they can both use
`sendmsg`.
Should I squash the commits first and then make the changes (add `send_vectored_to` etc.) or first the changes and then squash, so in the end, there would be 1 commit?
Squash first, then factor out a common function, then add send_vectored_to using that common function.
|
|
I think for datagrams this is not the right approach since you can only receive a datagram once and then if you want to inspect its headers you need to do this on the headers linked to this datagram. Having specialized methods to get, say... some fds or some other metadata means that querying this data is mutually exclusive. It would be better to create some opaque headers struct which can then be queried for additional information. I.e. a more general |
|
Okay, then I would not add the function to |
|
I focused on the datagram because the semantics are at least reasonably claer there. I'm not sure what the exact semantics of sending file descriptors over a SOCK_STREAM are, but the unix(7) says at least this: There's also this bit: The receiver is probably interested in the case where some FDs were discarded due to a too-small receive buffer. |
|
Sorry, I thought you mean So you suggest a more generic approach. |
|
Receiving the entire ancillary data may make sense, and then we can have helper functions to retrieve individual components of it, sure. (We could also have a function to get at the raw buffer.) I don't see any problem with initially just having parsing functions for fds, though. |
Well, if we add things piecemeal we have to maintain those specific methods. Doing it right once means less future baggage.
Not so much parsing, just not throwing away the raw ancillary data. Parsing can be added later if we have a struct that holds the data after the message has been received. |
|
The problem is, how big should the
This should be easy to implement because there are only three cases. |
|
Proposal: enum SocketAncillaryData {
FDs(Vec<RawFD>),
Cred(Vec<(u32, u32, u32)>),
SELinux(String)
}
fn recv_with_ancillary_data(
&self,
bufs: &mut [IoSliceMut<'_>],
msg_controllen: usize
) -> (usize, Vec<SocketAncillaryData>, SocketAddr);Would that be okay? |
|
The cmsg pattern is to let the caller allocate the storage, which makes sense if you call it frequently and want to avoid allocating every time. Same reason as for having the IoSlice. So maybe this would be better: struct SocketAncillaryData {
// opaque type
}
impl SocketAncillaryData {
pub fn new(capacity: usize) -> Self
// some of the data was lost due to insufficient capacity
// retrieved via MSG_CTRUNC flag from recvmsg
pub fn truncated() -> bool
pub fn get_fds(fds: &mut [usize]) -> usize
}
fn recv_with_ancillary_data(
&self,
bufs: &mut [IoSliceMut<'_>],
ancilliary: &mut SocketAncillaryData
) -> Result<(usize, SocketAddr), ...>;The recv method writes the raw data into the pre-allocated argument and then the caller can again extract the data into another preallocated buffer (to satisfy alignment requirements). At least that's seems to be the most direct translation of the C API. Parsing additional flags and other ancillary data could then be added later and expanded to UDP sockets, error messages and so on.
Well, you could do something with MSG_PEEK, but that's ugly. The truncation flag is the intended way to signal errors. |
|
Okay, @the8472 your proposal sound good. I would implement that way if @joshtriplett agrees. |
|
On Fri, Mar 13, 2020 at 07:57:21AM -0700, the8472 wrote:
The cmsg pattern is to let the caller allocate the storage, which makes sense if you call it frequently and want to avoid allocating every time. Same reason as for having the IoSlice.
So maybe this would be better:
```rust
struct SocketAncillaryData {
// opaque type
}
impl SocketAncillaryData {
pub fn new(capacity: usize) -> Self
// some of the data was lost due to insufficient capacity
// retrieved via MSG_CTRUNC flag from recvmsg
pub fn truncated() -> bool
pub fn get_fds(fds: &mut [usize]) -> usize
}
fn recv_with_ancillary_data(
&self,
bufs: &mut [IoSliceMut<'_>],
ancilliary: &mut SocketAncillaryData
) -> Result<(usize, SocketAddr), ...>;
```
The recv method writes the raw data into the pre-allocated argument and then the caller can again extract the data into another preallocated buffer (to satisfy alignment requirements).
At least that's seems to be the most direct translation of the C API.
Parsing additional flags and other ancillary data could then be added later and expanded to UDP sockets, error messages and so on.
That sounds reasonable to me.
|
|
Note that this was just a rough sketch of the idea. Some of the signatures could use further improvements. |
|
This is the first draft of the new interface b05873e71996b9085e5074007ae9f6657f49d1b8. I'm open to suggestions. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
If there no other suggestions, I would split the file |
|
That's an option. Another one would be to analyze where the tests could wait forever and which call might be causing that. If you're on linux and have docker installed you can also run the testsuite locally via I'm not sure how the emulator works, it's possible that it simply does not support the APIs you're trying to exercise and the tests have to deal with those error cases. |
|
Sorry that it took so long, but I did not have time. I probably found the bug, but it is not in my code. The bug is in the I will make a PR to fix the bug in |
|
The bug fix. |
|
It seems that the android implementation (bionic) is wrong. Therefore, I add a check for the android platform. |
|
It works now for android. I used the |
|
Perhaps add a comment why android needs special treatment. Or extend the commit message to explain it. Not sure which is approach preferred here. |
|
Add the comment, which explain it. |
|
@jyn514 can we try it again? |
|
Yup, thanks for the reminder! @bors r=Amanieu rollup=iffy |
|
📌 Commit 8983752 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finally, done 😅 |
|
Thanks so much for sticking with this ❤️ |
|
Why does |
|
Because the |
|
Thanks for the explanation. It seems that there is no case in which the buffers are actually written to in |
|
@lukaslihotzki Thanks for your support. Could you inform me when change the API because I am planing to add |
|
@lukaslihotzki Oh, I now see that you already make PR. |
Add the functions
recv_vectored_fdsandsend_vectored_fdstoUnixDatagramandUnixStream. With this functionsUnixDatagramandUnixStreamcan send and receive file descriptors, by usingrecvmsgandsendmsgsystem call.