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

Implement open file descriptor locks in fcntl #1195

Merged
merged 14 commits into from Apr 8, 2020
Merged

Conversation

andrenth
Copy link
Contributor

Hello

This PR updates libc to 0.2.68, which adds the F_OFD_* fcntl commands, and uses them in nix::fcntl::fcntl.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

You need to add a CHANGLOG entry. And if possible, you should add tests for the new feature.

@andrenth
Copy link
Contributor Author

I've added the CHANGELOG entry. I'll try to come up with a test. Does the CI run on Linux? I see only FreeBSD mentioned, and O_OFD_* is Linux-specific.

@asomers
Copy link
Member

asomers commented Mar 17, 2020

CI runs on quite a few platforms. Cirrus does FreeBSD, and Travis does Linux, OSX, NetBSD (build only), and Android (build only)

@andrenth
Copy link
Contributor Author

So, I've added #[cfg(not(target_arch = "mips"))] to the test because I can't seem to instantiate a libc::flock on mips, due to the pad field being private.

The failing tests in some architectures are caused by fcntl returning EINVAL, which means the F_OFD_SETLKW command is not recognized. This seems to be because of old kernel/libc versions in the test environments; for example, the tests do pass on a Debian Buster 32-bit VM.

Do you think it would be fine for now to simply not run this test for aarch64, arm, armv7, i686, mips64, mips64el and powerpc64?

@andrenth
Copy link
Contributor Author

Travis now only fails for i686. I don't know why that test is running despite the #[cfg(not(any(..., target_arch = "i686", ...)))].

#[cfg(not(any(target_arch = "aarch64",
target_arch = "arm",
target_arch = "armv7",
target_arch = "i686",
Copy link
Member

Choose a reason for hiding this comment

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

Try "x86" instead of "i686". Also, please explain in a comment why the test is disabled on certain architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment and switched to x86.

tmp.path().file_name().unwrap(),
OFlag::O_RDONLY,
Mode::empty()).unwrap();
let dirfd = open(tmp.path().parent().unwrap(), OFlag::empty(), Mode::empty()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you made some formatting changes unrelated to this PR. Could you please back those out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, forgot to disable rustfmt while editing. It's fixed.


// This test is disabled for the target architectures below
// due to OFD locks not being available in the kernel/libc
// versions used in the CI environment.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably related to the fact that CI for these platforms runs under QEMU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've amended the comment.

fcntl(fd, FcntlArg::F_OFD_SETLKW(&flock)).expect("write unlock failed");
assert_eq!(None, lock_info(inode));

flock.l_type = libc::F_RDLCK as libc::c_short;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this test is doing two separate things: setting and clearing a write lock, and setting and clearing a read lock. Please split it up into two separate tests. That will make it easier to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

LGTM!

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 8, 2020

Build succeeded

@bors bors bot merged commit b5ee610 into nix-rust:master Apr 8, 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.

None yet

2 participants