-
Notifications
You must be signed in to change notification settings - Fork 227
Add socket_peerpidfd
#1474
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
base: main
Are you sure you want to change the base?
Add socket_peerpidfd
#1474
Conversation
|
|
sunfishcode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, with one quick question:
| #[test] | ||
| #[serial] // for `setrlimit` usage, and `crate::init` | ||
| fn test_select_with_maxfd_sockets() { | ||
| use rustix::fd::{FromRawFd as _, OwnedFd}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why these lines were needed in the tests, that weren't otherwise changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That fixes a compile error with cargo test --features net,process,event. Though it also works to use --features=all-apis like CI uses. (I suppose ideally it shouldn't error for any combination of features; but doesn't matter that much for tests.)
|
Those imports are unfortunately evoking warnings, which are currently configured to be errors in CI. Could you add conditionals to the imports so that they're not imported if they're not needed? |
ab3c1ac to
896e94b
Compare
|
Huh, looking at CI, I guess Not sure why those are defined separately for Android; I'd assume unless the C standard library is doing something weird, those particular definitions should be the same for anything with a Linux kernel. |
|
@ids1024 Thanks so much for doing this. Do you intend to finish this PR? |
e7427e1 to
34551eb
Compare
|
I guess to pass CI this could just be disabled on Android. Presumably Tests seem to be broken with a recent compile, so I rebased this on #1523. Looks like there are a couple other unrelated CI errors? But I think this should be mergeable. |
e8980ab to
9f77ef6
Compare
|
Or well, one of the errors was related to this change. Fixed that, and added a commit for another CI error. So now everything is passing. |
db25968 to
e749200
Compare
|
I haven't investigated this yet myself, but CI is failing on a missing |
e749200 to
1cb61fb
Compare
Supported on Linux since 6.5.
1cb61fb to
f4a8327
Compare
|
I guess the last time I rebased this I did it on a different computer without pulling the latest version of the branch; so it lacked a fix to not build enable the feature on Android. Presumably Android should support this too, but that will require a PR to This should be mergeable now. |
Supported on Linux since 6.5.
I see
socket_peercredis only provided on Linux, while OpenBSD (https://man.openbsd.org/getsockopt.2) has some version ofSO_PEERCRED, and FreeBSD (https://man.freebsd.org/cgi/man.cgi?unix(4)) has aLOCAL_PEERCRED(libwayland for instance has code to use those on their respective platforms). These APIs do seem to be slightly different though. In any case,SO_PEERPIDFDis definitely Linux-specific for the time being.