Skip to content

linux: Add missing Linux-specific fcntls #233

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 1 commit into from
Mar 18, 2016

Conversation

kamalmarhubi
Copy link
Contributor

Also move F_DUPFD_CLOEXEC up a level as it is available on Android.

@rust-highfive
Copy link

r? @alexcrichton

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

@alexcrichton
Copy link
Member

@bors: r+ 104563f

@bors
Copy link
Contributor

bors commented Mar 17, 2016

⌛ Testing commit 104563f with merge 0e78d56...

bors added a commit that referenced this pull request Mar 17, 2016
linux: Add missing Linux-specific fcntls

Also move F_DUPFD_CLOEXEC up a level as it is available on Android.
@kamalmarhubi
Copy link
Contributor Author

Failures are all lack of F_ADD_SEALS and F_GET_SEALS. These are available in 3.17+ according to the man page: http://man7.org/linux/man-pages/man2/fcntl.2.html

Reading that, I realize I should have also added the F_SEAL_* constants, which I've now done.

@alexcrichton should I keep these constants in and modify the test code to ignore them?

@kamalmarhubi
Copy link
Contributor Author

Well, I modified test code to ignore them in the meantime in case you choose to r+ it. At least then tests should be passing when @bors gets to it. :-)

@posborne
Copy link
Contributor

Related to #228, I'm seeing if I can come up with a decent solution for verifying some constants against the kernel headers themselves. There are few challenges there:

  • Getting an appropriate version of the test headers into the test environment
  • Setting up defines so that the appropriate defines are in place for the targeted architecture such that the kernel definitions we compare against are accurate.
  • Ignoring the many things we probably don't want to check directly against the kernel.

I haven't done much in the way of implementation towards this goal as of yet, but my gut tells me that some modifications to ctest will be in order to only include (rather than exclude as is the case presently) certain symbols/patterns in what is tested.

Even if some of these things end up being definitions we want to include in dowstream crates, the ability to verify that constants and other items match the operating systems C definitions should be very useful.

@kamalmarhubi
Copy link
Contributor Author

@posborne I was thinking something along these lines, but going to generation rather than verification. This also fits in with #121 (adding all SYS_call constants for all platforms). Opened #234 to figure out an approach.

@posborne
Copy link
Contributor

Thanks for opening #234. I would be in favor of an approach using generation -- might be worth studying what is done in bionic.

@alexcrichton
Copy link
Member

I'd like to avoid adding constants to libc that are entirely untested in swaths like this, ignore a constant is basically a "massive hammer" to a solution.

If these aren't present in header files, how does C access these constants? That is, if we can't verify them, how can anyone use them?

@posborne
Copy link
Contributor

@alexcrichton The unfortunate answer for most C code is that you end up copying the header out of the kernel and into your application -- I have had to do this many times and it always bothered me. If you are lucky, there is a library (for example, libnl) that was developed along side the kernel functionality which provides headers for you.

Starting with kernel 3.7, there has been a push to separate the headers that are suitable for inclusion from userspace (or at least from libc) into the include/uapi and arch/$ARCH/include/uapi directories in the kernel. This is the case for the constants in this PR: http://lxr.free-electrons.com/source/include/uapi/linux/fcntl.h.

The idea has been around for awhile, but it recently got a bigger push. Pinging @mkerrisk to see if he has any advice in this regard for libc authors (in this case, in a non-C based language).

@alexcrichton
Copy link
Member

Interesting, thanks for the info @posborne! If those headers are explicitly intended to be included by userspace, perhaps this PR could start including that? Or does the travis builder not have those headers yet?

@kamalmarhubi
Copy link
Contributor Author

I didn't know too much about this, thanks for the links @posborne.

From reading a bit more, it appears that these uapi headers end up being installed under /usr/include/linux/. However, including that file results in conflicts with the original fcntl.h, which is included via several other headers. To get this to work, we'd have to make a bunch of changes to what headers get used in verification on Linux.

For the sake of this change, and the constants I actually want right now, I've deferred the file sealing fcntls and associated bitflag constants.

I opened #235 for the greater change to verifying against other headers.

Also move F_DUPFD_CLOEXEC up a level as it is available on Android.

This commit leaves file sealing related fcntls and bitflag constants
out, as they are defined in `linux/fcntl.h` rather than `fcntl.h`. They
can be included once an approach for verification has been figured out.
See rust-lang#235 for more detail.
@kamalmarhubi kamalmarhubi force-pushed the linux-fcntl branch 2 times, most recently from b43118c to 5c55ce0 Compare March 18, 2016 13:42
@kamalmarhubi
Copy link
Contributor Author

Testing including them in Android and musl. musl should definitely have them; not 100% sure of Android.

@kamalmarhubi
Copy link
Contributor Author

Androind and musl failed; reverted to omitting them.

@alexcrichton
Copy link
Member

@bors: r+ b43118c

@bors
Copy link
Contributor

bors commented Mar 18, 2016

⌛ Testing commit b43118c with merge cd925e0...

bors added a commit that referenced this pull request Mar 18, 2016
linux: Add missing Linux-specific fcntls

Also move F_DUPFD_CLOEXEC up a level as it is available on Android.
@bors
Copy link
Contributor

bors commented Mar 18, 2016

☀️ Test successful - status-appveyor, travis

@bors bors merged commit b43118c into rust-lang:master Mar 18, 2016
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