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

Android: Add VMADDR_ constants. #1913

Merged
merged 1 commit into from
Oct 7, 2020
Merged

Android: Add VMADDR_ constants. #1913

merged 1 commit into from
Oct 7, 2020

Conversation

qwandor-google
Copy link

No description provided.

@rust-highfive
Copy link

r? @JohnTitor

(rust_highfive has picked a reviewer for you, use r? to override)

@joshtriplett
Copy link
Member

Could you please move these up a level to src/unix/linux_like/mod.rs, so that they can be shared among targets? (Assuming that doing so builds on all linux_like targets.) If necessary, you could put them under cfg_if!.

@JohnTitor
Copy link
Member

Also, seems VMADDR_CID_LOCAL is newly added (torvalds/linux@ef343b3) so we should skip the tests for it.

@qwandor-google
Copy link
Author

qwandor-google commented Oct 7, 2020

There are many things which are the same between linux_like/android and linux_like/linux, as they are both actually Linux. The trouble is that emscripten is also under linux_like, and of course doesn't support all these sorts of things. If we moved emscripten up a level to unix instead then we could factor out a lot of common code from android and linux. Or alternatively, android could be moved under linux, as it is really just Linux with Bionic (rather than glibc or musl, which are both included under linux already).

This would be a fairly major refactor though. For now, it seems more consistent with the rest of the code to put things in both android/mod.rs and linux/mod.rs rather than having lots of cfg_if! in linux_like/mod.rs.

@joshtriplett
Copy link
Member

@qwandor-google I don't know if emscripten is getting much benefit from being under linux_like rather than just unix. On the one hand it might implement some things that Linux does but unix doesn't, in which case it'd be nice to avoid duplicating that. On the other hand, it does regularly seem like emscripten needs excluding like this from APIs which otherwise exist anywhere the Linux kernel does.

Separately from that, moving android under the linux directory (alongside gnu) seems like a great plan, and I'd love to see that. That seems like it'd avoid a lot of duplication, and it would also allow moving anything in linux_like that excludes emscripten down a level to linux to remove the corresponding cfg_if.

That's obviously not a prerequisite for this change. But if you'd be willing to work on that, I think that'd make it much easier to align APIs between the linux and android cases. @JohnTitor, what do you think?

@qwandor-google
Copy link
Author

@joshtriplett I don't know enough about emscripten to know what the right way forward is on that. I guess we should open an issue to track the refactoring in any case, and discuss it there rather than on this pull request.

The test failure above is only on s390x, and looks like a timeout unrelated to this change. I suggest re-running.

@JohnTitor
Copy link
Member

I don't know if emscripten is getting much benefit from being under linux_like rather than just unix. On the one hand it might implement some things that Linux does but unix doesn't, in which case it'd be nice to avoid duplicating that. On the other hand, it does regularly seem like emscripten needs excluding like this from APIs which otherwise exist anywhere the Linux kernel does.

Fair enough, but we should do that carefully not to break anything.

I don't know enough about emscripten to know what the right way forward is on that. I guess we should open an issue to track the refactoring in any case, and discuss it there rather than on this pull request.

Sounds good, opened #1915, and just added a brief explanation. I'd appreciate any comments :)

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failure above is only on s390x, and looks like a timeout unrelated to this change. I suggest re-running.

I'm going to approve as-is since it passed at the previous testing. Thanks!

@JohnTitor JohnTitor merged commit da0037c into rust-lang:master Oct 7, 2020
@qwandor-google qwandor-google deleted the vmaddr branch October 19, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants