Add support for 'posix_fallocate' #779

SanchayanMaity · 2017-10-09T09:01:13Z

No description provided.

asomers · 2017-10-11T02:24:09Z

src/fcntl.rs

+///
+/// Ensure disk space is allocated for the file referred to by the file descriptor
+/// fd for the bytes in the range starting at offset and continuing for len bytes.
+#[cfg(any(target_os = "linux"))]


This #[cfg()] statement is too narrow. In addition to Linux, libc already defines posix_fallocate for FreeBSD, Dragonfly, and Android. NetBSD has it too, though libc lacks the definition for NetBSD. Please broaden the definition. You will also need a matching #[cfg()] statement on the tests, or else nix's tests won't build on platforms that don't define this function.

Susurrus · 2017-10-18T04:24:27Z

@SanchayanMaity You still planning on working on this?

SanchayanMaity · 2017-10-18T04:51:54Z

@Susurrus Yes, I plan to work on this. Got busy with a few things last week. Will work on it today.

SanchayanMaity · 2017-10-26T05:48:25Z

How to fix the build for darwin?

asomers · 2017-11-05T17:18:11Z

The build failed on Darwin because libc doesn't define posix_fallocate for that platform. Either the definition is missing from libc or Darwin really doesn't have posix_fallocate. You need to figure out which. In the former case, send a PR to libc. In the latter, you just need to amend your #[cfg()] guard in this PR.

Susurrus · 2017-11-05T18:53:59Z

@asomers @SanchayanMaity I just checked on my mac running 10.11.6 and there is no definition of posix_fallocate, so macos and ios targets should definitely be skipped.

Also, this needs a rebase so that it'll run on all the current targets nix supports.

asomers · 2017-11-11T19:05:30Z

src/fcntl.rs

+///
+/// Ensure disk space is allocated for the file referred to by the file descriptor
+/// fd for the bytes in the range starting at offset and continuing for len bytes.
+#[cfg(any(target_os = "linux", target_os = "macos", target_os = "android", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd"))]


Please wrap long lines.

asomers · 2017-11-11T19:10:35Z

src/fcntl.rs

+/// Allocate file space.
+///
+/// Ensure disk space is allocated for the file referred to by the file descriptor
+/// fd for the bytes in the range starting at offset and continuing for len bytes.


Please add a link to http://pubs.opengroup.org/onlinepubs/9699919799/ .

asomers · 2017-11-11T19:11:49Z

test/test_fcntl.rs

+    let tmp = NamedTempFile::new().unwrap();
+
+    let fd = tmp.as_raw_fd();
+    posix_fallocate(fd, 0, 100).unwrap();


This test will fail if /tmp's filesystem doesn't support posix_fallocate. ZFS, for example, does not. POSIX stupidly requires that posix_fallocate return EINVAL in that case. So you should skip the rest of the test if you get that errno.

Susurrus · 2017-12-09T18:07:30Z

@SanchayanMaity Any time to wrap up this PR? It's pretty close to being able to be merged.

SanchayanMaity · 2017-12-10T15:43:24Z

@Susurrus Will pick this up again. Sorry for the delay. Was in between jobs.

asomers · 2018-01-17T01:10:57Z

src/fcntl.rs

+/// Allocate file space.
+///
+/// Ensure disk space is allocated for the file referred to by file
+/// descriptor fd for the bytes in the range starting at offset and


parameter names like fd and offset should be in backticks.

asomers · 2018-01-17T01:13:07Z

src/fcntl.rs

+/// Ensure disk space is allocated for the file referred to by file
+/// descriptor fd for the bytes in the range starting at offset and
+/// continuing for len bytes.
+/// http://pubs.opengroup.org/onlinepubs/9699919799/


The link is wrong. I think you probably linked to the wrong iframe. Also, for consistency with other Nix functions, you should format the link like this:

/// # References /// /// [`posix_fallocate`](http://whatever)

asomers · 2018-01-17T01:13:37Z

src/fcntl.rs

+/// descriptor fd for the bytes in the range starting at offset and
+/// continuing for len bytes.
+/// http://pubs.opengroup.org/onlinepubs/9699919799/
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd"))]


You still need to wrap long lines here and elsewhere.

This list should also be alphabetized.

Susurrus · 2018-01-17T02:37:55Z

src/fcntl.rs

+/// descriptor fd for the bytes in the range starting at offset and
+/// continuing for len bytes.
+/// http://pubs.opengroup.org/onlinepubs/9699919799/
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd"))]


This list should also be alphabetized.

Susurrus · 2018-01-17T02:40:39Z

test/test_fcntl.rs

+    let ret = posix_fallocate(fd, 0, 100).unwrap();
+
+    match Errno::result(ret) {
+       Err(Error::Sys(Errno::EINVAL)) => {


Clippy will get mad about this, a match where only 1 arm is checked. Please rewrite this using if ret.is_some(), but keep the comment there so it's clear what's going on.

Long-term it'd be better to actually check somehow before the test is run if this will work and skip the test otherwise. Adding a TODO here would be helpful for that.

Susurrus · 2018-01-17T02:40:51Z

test/test_fcntl.rs

@@ -143,3 +146,27 @@ mod linux_android {
        assert_eq!(100, read(fd, &mut buf).unwrap());
    }
 }
+
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd"))]


Please alphabetize this and line wrap it also.

Susurrus · 2018-01-17T02:41:03Z

test/test_fcntl.rs

@@ -1,10 +1,13 @@
+use nix::errno::{Errno};


Please remove unnecessary braces here and elsewhere.

Susurrus · 2018-01-28T18:41:54Z

@SanchayanMaity Do you have time to come back and finish up this PR in the near future? It's been a while since you last updated it.

Susurrus · 2018-01-30T05:42:23Z

You'll need to correct your CHANGELOG entries such that they end up in the UNRELEASED section.

asomers

All LGTM now. I'll wait to see if @Susurrus has any comments before I merge it.

Susurrus

A few minor changes then this should be good.

Susurrus · 2018-02-04T03:50:54Z

src/fcntl.rs

+
+/// Allocate file space.
+///
+/// Ensure disk space is allocated for the file referred to by file


"to by the file"

Susurrus · 2018-02-04T03:52:44Z

CHANGELOG.md

@@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]
+- Add nix::sys::posix_fallocate


This should be under the "Added" section and rephrased like "Added nix::sys::posix_fallocate". Please also list the platforms that it was added for since it wasn't all platforms nix supports.

Susurrus · 2018-02-04T03:58:07Z

src/fcntl.rs

+/// # References
+///
+/// [`posix_fallocate`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html)
+#[cfg(any(target_os = "android", target_os = "dragonfly", target_os = "freebsd",


posix_fallocate is not defined in libc for DragonFlyBSD and OpenBSD and I couldn't find it in the public sources for those OSes either. I think this and the tests should be removed for those platforms.

Susurrus · 2018-02-04T03:59:08Z

src/fcntl.rs

+/// [`posix_fallocate`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html)
+#[cfg(any(target_os = "android", target_os = "dragonfly", target_os = "freebsd",
+          target_os = "linux", target_os = "openbsd"))]
+pub fn posix_fallocate(fd: RawFd, offset: libc::off_t,


Please annotate this as #[inline] since it's a very small wrapper around the bare libc call.

Susurrus · 2018-02-12T01:01:31Z

@SanchayanMaity Want to take care of these comments so we can get this merged?

SanchayanMaity · 2018-02-14T04:38:05Z

@Susurrus Yes, of course. Been busy lately. I will send out a patch tomorrow addressing your feedback.

Susurrus · 2018-02-18T03:39:16Z

Please change the CHANGELOG entry to be under the current UNRELEASED section, as this didn't make it into the 0.10 release.

Then it LGTM. @asomers if you want another look-over, now's the time as I'll merge it as soon as this gets updated.

Susurrus · 2018-02-21T03:28:48Z

Please move your changelog entry under the "Added" section within the "[UNRELEASED]" section and squash these commits into 1. Then we'll merge!

asomers · 2018-02-27T16:37:37Z

test/test_fcntl.rs

+    let fd = tmp.as_raw_fd();
+    let ret = posix_fallocate(fd, 0, 100);
+    if ret.is_ok() {
+        // The test will fail if /tmp's filesystem doesn't support


You shouldn't ignore all errors, just EINVAL.

asomers · 2020-10-03T16:47:46Z

Superseded by #1105

asomers reviewed Oct 11, 2017

View reviewed changes

asomers requested changes Nov 11, 2017

View reviewed changes

asomers requested changes Jan 17, 2018

View reviewed changes

Susurrus suggested changes Jan 17, 2018

View reviewed changes

asomers approved these changes Feb 3, 2018

View reviewed changes

Susurrus suggested changes Feb 4, 2018

View reviewed changes

Susurrus approved these changes Feb 18, 2018

View reviewed changes

Add support for 'posix_fallocate'

57df0e6

asomers requested changes Feb 27, 2018

View reviewed changes

asomers closed this Oct 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for 'posix_fallocate' #779

Add support for 'posix_fallocate' #779

SanchayanMaity commented Oct 9, 2017

asomers Oct 11, 2017

Susurrus commented Oct 18, 2017

SanchayanMaity commented Oct 18, 2017

SanchayanMaity commented Oct 26, 2017

asomers commented Nov 5, 2017

Susurrus commented Nov 5, 2017

asomers Nov 11, 2017

asomers Nov 11, 2017

asomers Nov 11, 2017

Susurrus commented Dec 9, 2017

SanchayanMaity commented Dec 10, 2017

asomers Jan 17, 2018

asomers Jan 17, 2018

asomers Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus commented Jan 28, 2018

Susurrus commented Jan 30, 2018

asomers left a comment

Susurrus left a comment

Susurrus Feb 4, 2018

Susurrus Feb 4, 2018

Susurrus Feb 4, 2018

Susurrus Feb 4, 2018

Susurrus commented Feb 12, 2018

SanchayanMaity commented Feb 14, 2018

Susurrus commented Feb 18, 2018

Susurrus commented Feb 21, 2018

asomers Feb 27, 2018

asomers commented Oct 3, 2020

Add support for 'posix_fallocate' #779

Add support for 'posix_fallocate' #779

Conversation

SanchayanMaity commented Oct 9, 2017

Choose a reason for hiding this comment

Susurrus commented Oct 18, 2017

SanchayanMaity commented Oct 18, 2017

SanchayanMaity commented Oct 26, 2017

asomers commented Nov 5, 2017

Susurrus commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Dec 9, 2017

SanchayanMaity commented Dec 10, 2017

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

Susurrus commented Jan 28, 2018

Susurrus commented Jan 30, 2018

asomers left a comment

Choose a reason for hiding this comment

Susurrus 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

Susurrus commented Feb 12, 2018

SanchayanMaity commented Feb 14, 2018

Susurrus commented Feb 18, 2018

Susurrus commented Feb 21, 2018

Choose a reason for hiding this comment

asomers commented Oct 3, 2020