Skip to content

Conversation

abonander
Copy link
Contributor

Closes #148

I didn't touch the POSIX_MADV constants, those can be consolidated in another PR.

I also wrote a quick test to make sure that the call was written correctly and that posix_fadvise() actually works for my use-case:

https://gist.github.com/cybergeek94/ce5e989c6ac6823acea4

As you can see, the uncached file takes much longer to read without the call to posix_fadvise(), which is exactly the result I wanted to see. A test file size of 8M is around what I'll be dealing with in my particular use-case (large libraries of images).

cc @alexcrichton

@alexcrichton
Copy link
Member

lgtm, will merge once CI is green

@alexcrichton
Copy link
Member

Hm not entirely sure why travis didn't trigger here, perhaps a force push will fix that?

@abonander
Copy link
Contributor Author

$ git push --force
Everything up-to-date

Should I move some things around or something?

@alexcrichton
Copy link
Member

Ah I think if you do git commit --amend and don't actually change anything it'll basically touch a timestamp to allow you to force-push

@abonander
Copy link
Contributor Author

Done. I see the build has been triggered this time.

One possible concern is that posix_fadvise() is supposed to have a slightly different signature on ARM, as explained under the Notes section in the documentation. However, it appears it just means that calling the function as-is on ARM will use a couple extra registers because the arguments have to be aligned to 64 bits.

Also, the POSIX_FADV_DONTNEED and POSIX_FADV_NOREUSE constants have slightly different values on S/390 but considering that Rust does not support S/390 nor likely ever will, I figured it wouldn't be a problem.

@alexcrichton
Copy link
Member

Looks like this may not exist on OSX or on OpenBSD? Otherwise the signature seems to be agreed upon for supported arches

@abonander
Copy link
Contributor Author

Support appears to be inconsistent across BSD releases. It's in some but not in others, earlier or later. Should I just move it to notbsd then? FreeBSD does support it pretty consistently, though.

@alexcrichton
Copy link
Member

Ah I'd be fine either way

@abonander
Copy link
Contributor Author

notbsd covers my use case, but I find it weird that there wasn't a similar error for posix_madvise(). I guess the BSDs decided to support only that one?

@semarie
Copy link
Contributor

semarie commented Feb 7, 2016

yes, OpenBSD supports posix_madvise (see http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/madvise.2), but doesn't support posix_fadvise.

@abonander
Copy link
Contributor Author

Seems pretty arbitrary for OpenBSD but I'm fine moving this to notbsd and possibly freebsdlike so FreeBSD gets it.

@alexcrichton
Copy link
Member

Yeah that sounds good to me, just lemme know when it's pushed up!

@abonander abonander force-pushed the master branch 2 times, most recently from 861e11d to c80fc21 Compare February 13, 2016 07:22
@abonander
Copy link
Contributor Author

Moved function declarations and constants to relevant files, rebased to upstream, and squashed.

alexcrichton added a commit that referenced this pull request Feb 13, 2016
Add `posix_fadvise()` and related constants
@alexcrichton alexcrichton merged commit 213a0f9 into rust-lang:master Feb 13, 2016
@abonander
Copy link
Contributor Author

Cheers!

Susurrus pushed a commit to Susurrus/libc that referenced this pull request Mar 26, 2017
The returned length of AF_UNIX sockaddrs is significant, and generally
does not match the length of the entire structure. For filesystem
sockets, this is ignorable because the path is also NUL-terminated, but
for unbound sockets (e.g., a socketpair) or abstract-namespace
sockets (a Linux extension where the address is an arbitrary
bytestring), we need to keep track of the length.

Fixes rust-lang#177. Also add a UnixAddr::new_abstract function and some better
handling of abstract-namespace socket addresses to fix rust-lang#169.
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