posix_fallocate support #1105

dingxiangfei2009 · 2019-08-11T07:26:00Z

This PR add posix_fallocate, which is available on

Linux
FreeBSD
Android
Emscripten
Fuchsia
WASI

Here is a question: for some reason, posix_fallocate returns EBADF instead of EPIPE if a FIFO file descriptor is passed in on Linux 4.19.64. In the test EBADF is used for now, but I would like to know if such behaviour is expected.

asomers · 2019-08-11T14:38:58Z

It's not nix's job to fix kernel bugs; we have to live with them. You should adjust the test so that it accepts all error codes that the kernel might return in that situation.

dingxiangfei2009 · 2019-08-14T07:55:50Z

Sorry, forgot to cross reference here: CraneStation/wasi-common#16

asomers

Don't forget to add a CHANGELOG entry, too.

asomers · 2019-08-14T16:39:17Z

test/test_fcntl.rs

+        const LEN: usize = 100;
+        let mut tmp = NamedTempFile::new().unwrap();
+        let fd = tmp.as_raw_fd();
+        let res = posix_fallocate(fd, 0, LEN as libc::off_t).unwrap();


This test will fail if /tmp resides on a file system that doesn't support posix_fallocate, like ZFS. The test must be adjusted to handle that error. Unfortunately, POSIX stipulates that the errno in that case shall be EINVAL, which can also indicate several other problems.

Thanks! I wasn't aware of this case.

I have added in some explanation in the test, in case the test fails possibly due to filesystem not supporting this operation.

asomers · 2019-08-14T16:39:44Z

test/test_fcntl.rs

+        let (rd, _wr) = pipe().unwrap();
+        let errno = posix_fallocate(rd as RawFd, 0, 100).unwrap();
+        match Errno::from_i32(errno) {
+            Errno::EBADF


Can't you narrow this list down? I don't think there are very many different error codes that could be returned in this case.

asomers

Don't forget to add a CHANGELOG entry. And please don't squash until the end; it makes incremental review more difficult.

asomers · 2019-08-20T15:03:58Z

src/fcntl.rs

@@ -504,3 +504,16 @@ mod posix_fadvise {
        Errno::result(res)
    }
 }
+
+#[cfg(any(


You can add dragonfly, too. You can also add NetBSD, if you make a PR to libc first.

asomers · 2019-08-20T15:07:58Z

test/test_fcntl.rs

+            match Errno::from_i32(res) {
+                Errno::EINVAL => {
+                    eprintln!(r#"
+`posix_fallocate` returned EINVAL.


No need to print this info out; a comment is sufficient. And please keep it to 80 characters.

Okay. It is in comment now.

asomers · 2019-08-20T15:09:06Z

test/test_fcntl.rs

+    use nix::unistd::pipe;
+
+    #[test]
+    fn test_success() {


Putting "test" in the function name is redundant with the "test" in the module name. You can shorten it to "success".

asomers · 2019-08-20T15:11:24Z

src/fcntl.rs

+    any(target_os = "wasi", target_env = "wasi"),
+    target_env = "freebsd"
+))]
+pub fn posix_fallocate(fd: RawFd, offset: libc::off_t, len: libc::off_t) -> Result<libc::c_int> {


A better return type would be Result<()>, because the only non-error value ever returned is 0.

The spec says that posix_fallocate returns the error number and it does not set errno.
fallocate returns -1 on error and set errno. The two functions are behaving quite differently regarding to the returned value.

I have converted to return type to Result<()>. However, I have concerns on whether we should follow POSIX and return Errno directly?

posix_fallocate does what? That's bonkers. But sure enough; you're correct. However, we shouldn't expose that inconsistency to userspace. Better for Nix's posix_fallocate to have the same interface as other Nix functions. We should turn its return value into a Result<()>.

asomers · 2019-08-20T15:11:39Z

test/test_fcntl.rs

+    #[test]
+    fn test_errno() {
+        let (rd, _wr) = pipe().unwrap();
+        let errno = posix_fallocate(rd as RawFd, 0, 100).unwrap();


Shouldn't this be unwrap_err?

asomers · 2019-08-20T15:15:38Z

test/test_fcntl.rs

+          target_os = "emscripten",
+          target_os = "fuchsia",
+          any(target_os = "wasi", target_env = "wasi"),
+          target_env = "freebsd"))]


s/target_env/target_os/

asomers · 2019-08-20T15:15:52Z

src/fcntl.rs

+    target_os = "emscripten",
+    target_os = "fuchsia",
+    any(target_os = "wasi", target_env = "wasi"),
+    target_env = "freebsd"


s/target_env/target_os/

dingxiangfei2009 · 2019-08-25T16:28:58Z

@asomers Sorry for squashing again!

I will not do this for later commits.

Also, I am going to add an entry to the CHANGELOG, once my question about the function signature of posix_fallocate is resolved.

asomers · 2019-08-27T17:28:32Z

test/test_fcntl.rs

+                // According to POSIX.1-2008, its implementation shall
+                // return `EINVAL` if `len` is 0, `offset` is negative or
+                // the filesystem does not support this operation.
+                // According to POSIX.1-2001, its implementation shall


This comment is duped.

asomers · 2019-08-27T17:57:51Z

test/test_fcntl.rs

+                | Sys(Errno::EBADF) => (),
+            errno =>
+                panic!(
+                    "errno does not match posix_fallocate spec, errno={}",


I would simply say "unexpected errno, {}, errno

asomers · 2019-08-27T20:52:28Z

test/test_fcntl.rs

+                assert_eq!(&data as &[u8], &[0u8; LEN] as &[u8]);
+            }
+            Err(nix::Error::Sys(Errno::EINVAL)) => {
+                // `posix_fallocate` returned EINVAL.


Let me suggest some wording that would sound more natural to a native english speaker:

POSIX requires posix_fallocate to return EINVAL both for invalid arguments (i.e. len < 0) and if the operation is not supported by the file system. There's no way to tell for sure whether the file system supports posix_fallocate, so we must pass the test if it returns EINVAL.

Thank you for your suggestion! 😄

asomers · 2019-09-04T14:45:20Z

test/test_fcntl.rs

+            Ok(_) => {
+                let mut data = [1u8; LEN];
+                assert_eq!(tmp.read(&mut data).expect("read failure"), LEN);
+                assert_eq!(&data as &[u8], &[0u8; LEN] as &[u8]);


Instead of casting, you should be able to do &data[..], &[0u8; LEN][..]

asomers · 2019-09-04T14:46:31Z

CHANGELOG.md

@@ -773,3 +773,5 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#335](https://github.com/nix-rust/nix/pull/335))

 ## [0.5.0] 2016-03-01
+- Added `posix_fallocate`.


This CHANGELOG entry is seriously dislocated. It should be up in the "Unreleased" section.

dingxiangfei2009 · 2019-09-12T00:14:42Z

It is surprising to see tests failing under x86_64-unknown-linux-gnu.

asomers

Please rebase the PR onto master. Never merge master into PRs. As for the test failures, it looks like something on Travis's side changed.

asomers

bors r+

1105: posix_fallocate support r=asomers a=dingxiangfei2009 This PR add [`posix_fallocate`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html), which is available on - Linux - FreeBSD - Android - Emscripten - Fuchsia - WASI Here is a question: for some reason, `posix_fallocate` returns `EBADF` instead of `EPIPE` if a FIFO file descriptor is passed in on Linux 4.19.64. In the test `EBADF` is used for now, but I would like to know if such behaviour is expected. Co-authored-by: Ding Xiang Fei <[email protected]>

asomers · 2019-09-28T16:26:48Z

Sorry, but you need to rebase again to fix the test failures.

bors · 2019-09-28T16:33:11Z

Build succeeded

continuous-integration/travis-ci/push
FreeBSD 11.2

dingxiangfei2009 force-pushed the posix_fallocate branch 2 times, most recently from 50417ff to 0617c24 Compare August 12, 2019 16:39

asomers requested changes Aug 14, 2019

View reviewed changes

dingxiangfei2009 force-pushed the posix_fallocate branch from 0617c24 to bbd498d Compare August 20, 2019 14:50

asomers requested changes Aug 20, 2019

View reviewed changes

dingxiangfei2009 force-pushed the posix_fallocate branch 4 times, most recently from 39818d2 to cf1f4ae Compare August 25, 2019 05:36

asomers reviewed Aug 27, 2019

View reviewed changes

asomers requested changes Sep 4, 2019

View reviewed changes

dingxiangfei2009 requested a review from asomers September 11, 2019 23:30

asomers reviewed Sep 12, 2019

View reviewed changes

dingxiangfei2009 added 5 commits September 12, 2019 09:30

posix_fallocate

dcf57ba

better wording

75370ba

CHANGELOG update

1a45a4e

use slices for array comparison

a348806

use unrelease section in changelog

877afa1

dingxiangfei2009 force-pushed the posix_fallocate branch from 481b162 to 877afa1 Compare September 12, 2019 01:31

dingxiangfei2009 requested a review from asomers September 23, 2019 02:29

asomers approved these changes Sep 28, 2019

View reviewed changes

bors bot merged commit 877afa1 into nix-rust:master Sep 28, 2019

dingxiangfei2009 deleted the posix_fallocate branch September 30, 2019 12:14

asomers mentioned this pull request Oct 3, 2020

Add support for 'posix_fallocate' #779

Closed

posix_fallocate support #1105

posix_fallocate support #1105

Conversation

dingxiangfei2009 commented Aug 11, 2019

asomers commented Aug 11, 2019

dingxiangfei2009 commented Aug 14, 2019

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 Aug 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 commented Aug 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiangfei2009 commented Sep 12, 2019

asomers left a comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment

asomers commented Sep 28, 2019

bors bot commented Sep 28, 2019

Build succeeded

dingxiangfei2009 Aug 25, 2019 •

edited

Loading

dingxiangfei2009 commented Aug 25, 2019 •

edited

Loading