Implementation of peer credentials for Unix sockets #75148
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (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
|
Awesome, thanks for the credit and for your additional work on getting this to std! Looking forward to another opportunity when I can use it. :) |
There was a problem hiding this comment.
something_t types are not used in std often (at all?), is this a good idea?
Personally I'd love seeing use of newtypes as done in nix, but not sure if that should go to std.
There was a problem hiding this comment.
I did a quick grep of the codebase and I can see a few places where these are used... I'll leave it as it is for now! If you would strongly like this to change feel free to follow up. 🙂
There was a problem hiding this comment.
I don't think assume_init is correct here. I believe raw ptr should be obtained from MaybeUninit, then passed to getsockopt and only after it succeeds calling assume_init
There was a problem hiding this comment.
Done -- I opted against using MemUninit in the end, it doesn't seem necessary.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Same issue with assume_init here
There was a problem hiding this comment.
Oooh, nice defensive programming using 1 instead of 0! ❤️
There was a problem hiding this comment.
Out of curiosity - why not return the process ID too, since that's returned by the syscall?
There was a problem hiding this comment.
PID is only available on Linux, it's not available on BSD.
There was a problem hiding this comment.
Is it possible to include it (conditionally) on Linux?
There was a problem hiding this comment.
Technically yes. It'd require change of the API due to coding standard in std (mainly using extension traits for additional platforms, IDK why such standard exists).
There was a problem hiding this comment.
Ah, fair enough, I was hoping for a small, clean change :) nevermind then
There was a problem hiding this comment.
Could we make the PID an Option<pid_t>? A quick search reveals that it is in fact supported on quite a few BSD platforms (e.g. FreeBSD).
There was a problem hiding this comment.
Done -- however, my Google-fu is failing me here. I can't find anything about retrieving the PID from a domain socket on FreeBSD. The closest I can find is getpeereid which accepts a UID and GID only. So at the moment, the optional PID is set to None for BSD, and Some(pid) on Linux.
There was a problem hiding this comment.
It seems to have been added relatively recently: https://svnweb.freebsd.org/base?view=revision&revision=348419
|
Hi all! Just gently pinging this PR for review. 🙂 |
crlf0710
added
S-waiting-on-review
|
@joechrisellis thanks but would advise not to ping for reviewers. The triage team currently uses the last time a PR was updated as a parameter to filter which pull requests need to be checked, which means posting a message that doesn't contribute to the PR can delay the process further. In future, if anyone reading this wants to grab attention on a PR that's sitting idle for a while, you can contact the triage-team on zulip and we will take care of it |
|
r? @Amanieu |
joechrisellis
force-pushed
the
master
branch
2 times, most recently
from
d1e209f to
318f84e
Compare
There was a problem hiding this comment.
Could we get doc comments for each of the public fields? In particular the Option<pid_t> should be explained in the docs.
|
@bors r+ |
|
📌 Commit 80860181445f14e6f305c687b2056f11ebc4629a has been approved by |
bors
removed
the
S-waiting-on-review
|
📌 Commit 68ff495 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
14 participants
The code in
ucred.rsis based on the work done in PR 13 in the tokio-uds repository on GitHub.This commit is effectively a port to the stdlib, so credit to Martin Habovštiak (@Kixunil) and contributors for the meat of this work. 🥇
Happy to make changes as needed. 🙂