Skip to content

libc-test's C program should not #include <linux/fs.h> on Linux #127

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 4 commits into from
Jan 12, 2016

Conversation

jimblandy
Copy link
Contributor

This is a fix for issue #113.

In libc-test/build.rs, the main function includes the directive #include <linux/fs.h> in the generated all.c file, but the correct header for system calls like mount is <sys/mount.h>; the <linux/fs.h> is just copied over from the kernel, and does not match the API offered by libc.

In particular:

  • If you happen to include <linux/fs.h> before <sys/mount.h>, you'll get compilation errors, because the former defines the same constants using preprocessor macros that the latter defines as enum values. The generated all.c file just happens to include them in the other order.
  • The <linux/fs.h> header includes a number of constants that are only meant to be used internal to the kernel, like MS_NOSEC and MS_BORN.
  • The <linux/fs.h> header defines MS_VERBOSE, which is 1) a perverse name, because it silences some printk messages, rather than enabling them, and 2) is superseded by MS_SILENT, according to both the man page for mount and comments in the <linux/fs.h> header itself.

This pull request removes the libc crate's definitions for MS_VERBOSE, MS_NOSEC, and MS_BORN, and then removes the directive to include <linux/fs.h>.

This fixes issue #113 because the libc crate no longer sees the inappropriate-for-userspace definition of MS_RMT_MASK.

@jimblandy
Copy link
Contributor Author

Oh, this pull request builds on pull request #126.

@jimblandy
Copy link
Contributor Author

The MIPS build failed; it seems like the libc we're using in the MIPS QEMU doesn't have some of the newer constants. The mount(2) man page says:

The definitions of MS_DIRSYNC, MS_MOVE, MS_REC, MS_RELATIME, and MS_STRICTATIME were added to glibc headers in version 2.12.

Those are among the identifiers Travis reports missing.

@polachok
Copy link
Contributor

polachok commented Jan 5, 2016

@jimblandy #114 may fix that

@jimblandy
Copy link
Contributor Author

@polachok: YAY

@alexcrichton
Copy link
Member

Could this be separated from #126? (will help landing)

Also looks like there's a number of CI failures, not all of which are spurious

@jimblandy
Copy link
Contributor Author

Could this be separated from #126? (will help landing)

Yes. (I had them on the same branch because I was trying to get a clean libc-test run, but in retrospect that's not the most helpful way to submit them.)

Also looks like there's a number of CI failures, not all of which are spurious

The ones I see are either caused by being built on #126, or are those I mentioned in comment 8, and should be fixed by #114.

@jimblandy
Copy link
Contributor Author

I took a look at #114; this pull request will need revision if that one lands first, and vice versa.

In the process of updating libc for the MIPS image, #114 makes a bunch of changes to the libc crate itself, in order to get a clean libc-test run. But if we stop including <linux/fs.h> as I'm advocating in this PR, some of those changes will need to come back out.

If we want both PRs, I'm happy to revise this one after #114 is merged. I don't know how often the libc crate gets published to crates.io, but we should take care not to publish when only one has been merged, to avoid churn.

@alexcrichton
Copy link
Member

@jimblandy ah ok, I've merged #114 now (to fix all the mips build failures), and if you want to rebase this it should be all green and good to go.

Perhaps we should perform an audit and remove all inclusions of linux header files?

@jimblandy
Copy link
Contributor Author

The commit 53bf018 died on MIPS because the definition of MS_RMT_MASK had become the same as all the others. I fixed that, and added a commit that commons up all the MS_RMT_MASK definitions to a single one common to all Linux-kernel based targets, which makes more sense.

alexcrichton added a commit that referenced this pull request Jan 12, 2016
libc-test's C program should not #include <linux/fs.h> on Linux
@alexcrichton alexcrichton merged commit 4d9ab7e into rust-lang:master Jan 12, 2016
@alexcrichton
Copy link
Member

Thanks!

danielverkamp pushed a commit to danielverkamp/libc that referenced this pull request Apr 28, 2020
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.

3 participants