-
Notifications
You must be signed in to change notification settings - Fork 346
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
Added epoll and eventfd for Android #4016
base: master
Are you sure you want to change the base?
Conversation
// eventfd is Linux specific. | ||
this.assert_target_os("linux", "eventfd"); | ||
|
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.
If the intend is to allow eventfd on linux and android only, instead of removing the assert, maybe we could use
if !matches!(&*this.tcx.sess.target.os, "linux" | "android") {
panic!("`eventfd` should not be called on {}", this.tcx.sess.target.os);
}
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.
The thing here is that the eventfd
implementation resides in the linux
module, so I see no reason in that assertion. If it would be for any unix
with some exceptions, then yes, there's a good reason to keep the assertion.
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.
@RalfJung, what's your opinion?
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.
Seems like maybe it's a good time to introduce a unix::linux_like
module, and then put some things there that both unix::android
and unix::linux
can import and use as helpers.
I agree that in that module, if things work on all linux-like systems, they don't need a further assertion.
@rustbot author |
23c7c4b
to
731494f
Compare
@rustbot review |
☔ The latest upstream changes (presumably #4029) made this pull request unmergeable. Please resolve the merge conflicts. |
731494f
to
1f5c4e5
Compare
☔ The latest upstream changes (presumably #3939) made this pull request unmergeable. Please resolve the merge conflicts. |
1f5c4e5
to
a27dced
Compare
Fixes #3996.
I decided not to move
check_shim
into the implementations because all these functions are no more than just shims oversyscall
. Moreover,eventfd
is even used by thesyscall
implementation.