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

Added epoll and eventfd for Android #4016

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

YohDeadfall
Copy link
Contributor

Fixes #3996.

I decided not to move check_shim into the implementations because all these functions are no more than just shims over syscall. Moreover, eventfd is even used by the syscall implementation.

Comment on lines -185 to -149
// eventfd is Linux specific.
this.assert_target_os("linux", "eventfd");

Copy link
Contributor

@tiif tiif Nov 7, 2024

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);
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2024

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Nov 9, 2024
@YohDeadfall
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 11, 2024
@bors
Copy link
Contributor

bors commented Nov 11, 2024

☔ The latest upstream changes (presumably #4029) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 13, 2024

☔ The latest upstream changes (presumably #3939) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 22, 2024

☔ The latest upstream changes (presumably #4049) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: Add support for epoll, eventfd
5 participants