Skip to content

Add MODULE_INIT_ constants #1041

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

Merged
merged 5 commits into from
Jul 18, 2018
Merged

Add MODULE_INIT_ constants #1041

merged 5 commits into from
Jul 18, 2018

Conversation

bachp
Copy link
Contributor

@bachp bachp commented Jul 15, 2018

These are flags required to implement the linux kernel loading mechanism.
Specifically finit_module.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

@bachp
Copy link
Contributor Author

bachp commented Jul 15, 2018

I'm not sure if this constants should also be added to the android platform as the kernel module loading should be the same there.

@alexcrichton
Copy link
Member

It looks like tests are failing? Perhaps a missing header? It should be fine to exclude the constants on platforms where they aren't needed

@bachp
Copy link
Contributor Author

bachp commented Jul 16, 2018

That platforms where it is failing is Linux and Linux is the only one it is available. So excluding doesn't make much sense 😉

The constants are defined in the linux uapi header file module.h. Do I need to do something to makes this file known to the build?

@alexcrichton
Copy link
Member

You may need to tweak libc-test/build.rs to include the right header file

@bachp bachp force-pushed the module-init branch 3 times, most recently from 0b4b184 to a98a6ed Compare July 16, 2018 19:07
@bachp
Copy link
Contributor Author

bachp commented Jul 16, 2018

@alexcrichton It looks like the musl kernel-headers do not include the linux/module.h file. The issue seems to be similar to: #616

I'm unsure how to handle this. As this are only constant definitions that will be used later in a syscall (not a c function) from the nix crate. So I don't just want to exclude musl here.

I tried manually copying the module.h file to the right location, and this works, but feels like a very dirty hack.

I see several ways to fix this:

  1. Add the module.h to the kernel headers (very hacky)
  2. Don't define the constants in libc but in nix (seems to be not the proper way)
  3. Exclude musl (makes me unable to use them in nix when building against musl)

Any ideas?

@alexcrichton
Copy link
Member

The docker images we're using I think are copying the kernel heders from #616 I think? Are newer versions of the headers needed though?

@bachp
Copy link
Contributor Author

bachp commented Jul 17, 2018

Newer version of the "sanetized headers" are not available. I will check if I can bring the missing header in there and then use the newer version.

@bors
Copy link
Contributor

bors commented Jul 17, 2018

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

bachp added 2 commits July 17, 2018 18:41
These are flags required to implement the linux kernel loading mechanism.
Specifically finit_module.
@bachp bachp force-pushed the module-init branch 3 times, most recently from 0cb5699 to 3ce792b Compare July 17, 2018 20:47
@bachp
Copy link
Contributor Author

bachp commented Jul 17, 2018

Sparc64 is still failing as debian doesn't ship the required header file. Ubuntu does that's why it is working there.

@bachp
Copy link
Contributor Author

bachp commented Jul 17, 2018

@alexcrichton I got CI to pass but with two caveats:

  1. The musl build depend if Add missing linux/module.h sabotage-linux/kernel-headers#14 getting accepted to be properly working.
  2. The sparc64 build needs a hack to copy over the linux/module.h from the kernel headers uapi as it is not included in Debian.

@alexcrichton
Copy link
Member

This looks great, thanks! I think doing something weird for one platform is fine. It looks like upstream is merged so should the urls switch back to that?

@bachp
Copy link
Contributor Author

bachp commented Jul 18, 2018

@alexcrichton Yes I will update the PR

@bachp
Copy link
Contributor Author

bachp commented Jul 18, 2018

@alexcrichton Urls are switched back to upstream. I think this is good to go now.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 18, 2018

📌 Commit 2365707 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 18, 2018

⌛ Testing commit 2365707 with merge 72b16d2...

bors added a commit that referenced this pull request Jul 18, 2018
Add MODULE_INIT_ constants

These are flags required to implement the linux kernel loading mechanism.
Specifically finit_module.
@bors
Copy link
Contributor

bors commented Jul 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 72b16d2 to master...

@bors bors merged commit 2365707 into rust-lang:master Jul 18, 2018
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.

4 participants