Implement sendfile on FreeBSD and Darwin #901

morrowa · 2018-05-16T04:15:19Z

This PR exposes the sendfile system call on libc's supported BSD-likes:

FreeBSD
Darwin (macOS/iOS)

DragonFly could be supported in the future, but I was unable to build rustc to test.

Note that NetBSD has no equivalent system call.

asomers

In addition to the comments I made inline, please add a CHANGELOG entry.

asomers · 2018-05-16T14:17:06Z

src/sys/sendfile.rs

+#[allow(missing_debug_implementations)]
+#[allow(missing_copy_implementations)]
+#[repr(C)]
+pub struct SfHdTr(libc::sf_hdtr);


You should add a PhantomData field to keep sf_hdtr's pointers from dangling. See AioCb::from_mut_slice in src/sys/aio.rs for an example.

Done! Does it look correct?

asomers · 2018-05-16T14:18:04Z

src/sys/sendfile.rs

+    pub struct SfFlags: c_int {
+        SF_NODISKIO;
+        SF_MNOWAIT;
+        // SF_NOCACHE; // TODO: not yet exported by libc


Have you opened a PR with libc for SF_NOCACHE support? If not, you should. It's very easy to get PRs accepted to libc. When you do, you should also include SF_USER_READAHEAD, which is new in FreeBSD 12.

Working on a PR against libc now. The tests fail when I add the FreeBSD 12 constant and run them on my FreeBSD 11.1 machine. Once I figure that out, I'll submit it.

That's because libc's tests run on FreeBSD 11.1. If you're adding a 12.0-specific symbol, you'll have to add it to the skip_fn section of libc-test/build.rs.

rust-lang/libc#988

asomers · 2018-05-16T14:24:58Z

src/sys/sendfile.rs

+libc_bitflags!{
+    pub struct SfFlags: c_int {
+        SF_NODISKIO;
+        SF_MNOWAIT;


SF_MNOWAIT is obsolete in FreeBSD 11.0 and above. The last release that makes use of it, 10.4, goes EoL on Oct 31 of this year. So I'd rather not expose this symbol in Nix unless there is specific demand.

asomers · 2018-05-16T14:29:08Z

src/sys/sendfile.rs

+}
+
+#[cfg(target_os = "freebsd")]
+pub fn sendfile(in_fd: RawFd,


Could you please add some usage documentation?

asomers · 2018-05-16T14:41:45Z

test/test_sendfile.rs

+    let (mut rd, wr) = UnixStream::pair().unwrap();
+
+    // Call the test method
+    let (res, bytes_written) = sendfile(tmp.as_raw_fd(), wr.as_raw_fd(), body_offset as off_t, 0, Some(&hdtr), SfFlags::empty(), 0);


Wrap to 80 columns, please.

Should I just run rustfmt on the files I've touched?

asomers · 2018-05-16T14:43:34Z

src/sys/sendfile.rs

+pub fn sendfile(in_fd: RawFd,
+                out_sock: RawFd,
+                offset: off_t,
+                count: usize,


It would be more Rusty for count to be an Option<usize> instead of giving a special meaning to 0.

morrowa · 2018-05-16T16:45:01Z

Thanks for the very quick feedback! I'll update this PR soon.

Susurrus · 2018-05-16T22:32:21Z

src/sys/sendfile.rs

+#[allow(missing_debug_implementations)]
+#[allow(missing_copy_implementations)]
+#[repr(C)]
+pub struct SfHdTr(libc::sf_hdtr);


I'd love to see a better name for this struct compared to the cryptic shorthand libc uses. SendfileHeader I think would work

How about SendfileHeaderTrailer?

asomers · 2018-05-19T16:45:36Z

src/sys/sendfile.rs

+            pub struct SfFlags: c_int {
+                SF_NODISKIO;
+                SF_SYNC;
+                // SF_USER_READAHEAD; // TODO: waiting on libc release


Nix uses a git dependency on libc (except when we release), so you can go ahead and enable these now.

asomers · 2018-05-19T16:48:21Z

src/sys/sendfile.rs

+#[allow(missing_debug_implementations)]
+#[allow(missing_copy_implementations)]
+#[repr(C)]
+pub struct SfHdTr(libc::sf_hdtr);


asomers · 2018-05-19T16:51:05Z

I would still like to see some doc comments and a CHANGELOG entry. I would consider Dragonfly support to be optional, since we don't test that platform anyway.

morrowa · 2018-05-19T17:06:30Z

I haven't been able to build rustc on DragonFly after several tries, so I will not implement that in this PR. I'll add the doc comments, add the change log entry, clean up my TODOs, rebase, and ping you when it's ready. Thanks again!

morrowa · 2018-05-21T04:25:55Z

@asomers and @Susurrus this is ready for re-review! I believe it's complete.

asomers · 2018-05-21T12:24:10Z

src/sys/sendfile.rs

+
+        libc_bitflags!{
+            pub struct SfFlags: c_int {
+                SF_NODISKIO;


Could you please add some documentation for the flags?

asomers · 2018-05-21T12:24:36Z

src/sys/sendfile.rs

+///
+/// `in_fd` must support `mmap`-like operations and therefore cannot be a socket.
+///
+/// For more information, see `man 2 sendfile`


This should be a link, like other man pages references throughout our docs.

asomers · 2018-05-21T12:25:46Z

src/sys/sendfile.rs

+        /// included in the returned count of bytes written. The values of `offset` and `count` do
+        /// not apply to headers or trailers.
+        ///
+        /// `readahead` specifies how many pages may be cached in memory ahead of the page currently


s/how many pages may be cached/the minimum number of pages to cache/

asomers · 2018-05-21T12:29:50Z

test/test_sendfile.rs

+    tmp.write_all(body.as_bytes()).unwrap();
+
+    // Prepare headers and trailers for sendfile
+    let headers: Vec<IoVec<&[u8]>> = header_strings


Would it make sense for the API to accept a slice of slices instead of a slice of IoVecs? That would simplify the user's code. In fact, if you did this, then SendfileHeaderTrailer wouldn't even need to be public.

How about a slice of byte slices? I feel like it makes the API even less obtuse.

morrowa · 2018-05-22T04:36:19Z

@asomers ready for another look!

asomers · 2018-05-22T12:42:14Z

src/sys/sendfile.rs

-        pub struct SendfileHeaderTrailer<'a>(libc::sf_hdtr, PhantomData<&'a [IoVec<&'a [u8]>]>);
+        struct SendfileHeaderTrailer<'a>(
+            libc::sf_hdtr,
+            Option<Vec<IoVec<&'a [u8]>>>,


Why is it necessary to store the Option<Vec< fields in SendfileHeaderTrailer?

Because it's creating the IoVec structs from byte slices, it's now the logical owner of those structs.

It still doesn't own the data, however. PhantomData should be sufficient to keep the caller from dropping the byte slices. Or is there something I'm missing?

It doesn't own the byte slices, but it does own all of the IoVec instances, which do occupy memory.

Ahh, that makes sense. In that case, this PR LGTM. All it needs is a squash.

asomers

Approved! I'll leave it up for a few days, in case @Susurrus has any comments.

Susurrus

Just a few minor style fixups on my end. r+ once these are all corrected.

Susurrus · 2018-05-28T21:58:54Z

src/sys/sendfile.rs

+    }
+}
+
+cfg_if! {


This doesn't need a new one I don't think and could be bundled into the above macro, yes?

The header/trailer struct is the same across FreeBSD and Darwin, but the flags and function signatures are different, so they can't share all code. I could nest the ifs, but this felt like it caused less rightward drift.

I was referring only to the cfg_if! macro instance. Can you only have one if/else chain within each macro instance?

Yeah, if I try to put multiple independent if/else chains inside a single macro block, it won't compile. :(

Susurrus · 2018-05-28T21:59:37Z

src/sys/sendfile.rs

+/// `in_fd` must support `mmap`-like operations and therefore cannot be a socket.
+///
+/// For more information, see [the sendfile(2) man page.](http://man7.org/linux/man-pages/man2/sendfile.2.html)
+#[cfg(any(target_os = "linux", target_os = "android"))]


Please alphabetize the OSes listed here.

Susurrus · 2018-05-28T22:00:30Z

test/test_sendfile.rs


+#[cfg(any(target_os = "linux", target_os = "android"))]


Please alphabetize the target OS list here.

Susurrus · 2018-05-28T22:00:38Z

test/test_sendfile.rs

-use nix::unistd::{close, pipe, read};
-use nix::sys::sendfile::sendfile;
+cfg_if! {
+    if #[cfg(any(target_os = "linux", target_os = "android"))] {


Please alphabetize the target OS list here.

Susurrus · 2018-05-28T22:04:03Z

test/test_sendfile.rs

+    if #[cfg(any(target_os = "linux", target_os = "android"))] {
+        use nix::unistd::{close, pipe, read};
+    }
+    else if #[cfg(any(target_os = "freebsd", target_os = "ios", target_os = "macos"))] {


Please put else expressions on the same line as the closing brace as suggested by the style guidelines.

Susurrus · 2018-05-28T22:04:23Z

src/sys/sendfile.rs

+            (Errno::result(return_code).and(Ok(())), bytes_sent)
+        }
+    }
+    else if #[cfg(any(target_os = "ios", target_os = "macos"))] {


Please put else expressions on the same line as the closing brace as suggested by the style guidelines.

morrowa · 2018-05-28T23:31:47Z

@Susurrus style comments addressed! Ready for another look.

Susurrus · 2018-05-28T23:59:32Z

bors r+

901: Implement sendfile on FreeBSD and Darwin r=Susurrus a=morrowa This PR exposes the `sendfile` system call on libc's supported BSD-likes: * FreeBSD * Darwin (macOS/iOS) DragonFly could be supported in the future, but I was unable to build rustc to test. Note that NetBSD has no equivalent system call. Co-authored-by: Andrew Morrow <[email protected]>

bors · 2018-05-29T00:40:14Z

Build failed

continuous-integration/travis-ci/push

asomers · 2018-05-29T01:46:38Z

The test failure is weird. It looks like Nix's code is wrong, and AFAICT this should've been failing ever since 5378945, but it never failed until now. There have been no recent changes in either libc or japaric/cross that look related. I don't know why it just began to fail.

morrowa · 2018-05-29T01:46:48Z

A recent PR merged into libc changed the libc::bind function signature for 64-bit Android. That's what's causing the build failure. I'm quite confident it's unrelated to these changes.

asomers · 2018-05-29T02:00:23Z

You're right; I just couldn't find that with git blame for some reason.

Susurrus · 2018-05-29T02:43:24Z

Please see #906 for a fix.

@asomers

906: Fix bind() on Android 64-bit r=Susurrus a=Susurrus libc fixed `bind()` for Android 64-bit targets, so change our code to match. PRs are failing (like #901) so let's get this merged. CC @asomers @morrowa Co-authored-by: Bryant Mairs <[email protected]>

Susurrus · 2018-05-29T03:54:30Z

bors r+

901: Implement sendfile on FreeBSD and Darwin r=Susurrus a=morrowa This PR exposes the `sendfile` system call on libc's supported BSD-likes: * FreeBSD * Darwin (macOS/iOS) DragonFly could be supported in the future, but I was unable to build rustc to test. Note that NetBSD has no equivalent system call. Co-authored-by: Andrew Morrow <[email protected]>

bors · 2018-05-29T04:50:12Z

Build succeeded

asomers requested changes May 16, 2018

View reviewed changes

Susurrus suggested changes May 16, 2018

View reviewed changes

asomers reviewed May 19, 2018

View reviewed changes

morrowa changed the title ~~WIP: Implement sendfile on BSD-likes~~ Implement sendfile on FreeBSD and Darwin May 21, 2018

asomers reviewed May 21, 2018

View reviewed changes

asomers reviewed May 22, 2018

View reviewed changes

asomers approved these changes May 23, 2018

View reviewed changes

Susurrus suggested changes May 28, 2018

View reviewed changes

Susurrus approved these changes May 28, 2018

View reviewed changes

Susurrus mentioned this pull request May 29, 2018

Fix bind() on Android 64-bit #906

Merged

Implement sendfile on FreeBSD and Darwin

325c43c

bors bot merged commit 325c43c into nix-rust:master May 29, 2018

morrowa mentioned this pull request Jan 20, 2023

Implement sendfile for OS X / MacOS #560

Closed

Implement sendfile on FreeBSD and Darwin #901

Implement sendfile on FreeBSD and Darwin #901

Conversation

morrowa commented May 16, 2018 • edited Loading

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

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

morrowa commented May 16, 2018

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 commented May 19, 2018

morrowa commented May 19, 2018

morrowa commented May 21, 2018

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

morrowa commented May 22, 2018

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

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

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

morrowa commented May 28, 2018

Susurrus commented May 28, 2018

bors bot commented May 29, 2018

Build failed

asomers commented May 29, 2018

morrowa commented May 29, 2018

asomers commented May 29, 2018

Susurrus commented May 29, 2018

Susurrus commented May 29, 2018

bors bot commented May 29, 2018

Build succeeded

morrowa commented May 16, 2018 •

edited

Loading