Skip to content

Add copy_file_range() for Linux and Android #1476

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

Closed
wants to merge 1 commit into from

Conversation

ArniDagur
Copy link
Contributor

@ArniDagur ArniDagur commented Aug 17, 2019

This is my first PR to libc, so I looked at how splice() is implemented and modelled it after that.

See http://man7.org/linux/man-pages/man2/copy_file_range.2.html

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 17, 2019

When was this function added and in which header is it defined?

You might need to update libc-test/build.sh to only test it when the appropriate header is included, but it might be that the linux and android versions used in CI aren't new enough.

@ArniDagur
Copy link
Contributor Author

copy_file_range is supported in kernels 4.5 and above. It is a part of unistd.h in glibc. A mistake in this PR relates to the fact that there's only a wrapper present in glibc, but not musl.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 18, 2019

I see, well let me know when CI is green.

@ArniDagur
Copy link
Contributor Author

ArniDagur commented Sep 3, 2019

Just now, copy_file_range support for musl got merged. Does that mean it's fine to include it here immediately?

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 7, 2019

That would require upgrading the musl versions used here on CI.

@bors
Copy link
Contributor

bors commented Sep 13, 2019

☔ The latest upstream changes (presumably #1510) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 21, 2019

This needs rebasing

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 28, 2019

@ArniDagur we have recently upgraded the musl version, would you mind rebasing this to see if it passes tests and can be merged? Let me know if there is anything I can help you with!

@ArniDagur
Copy link
Contributor Author

@gnzlbg The musl tests still fail.

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 3, 2019

That probably means that the musl version used by CI still does not support this API.

@mati865
Copy link
Member

mati865 commented Dec 3, 2019

It was added in 1.1.24 (the same as on the CI) but it requires _GNU_SOURCE.
IIUC it's not enabled on purpose:

cfg.define("_GNU_SOURCE", None);

@JohnTitor
Copy link
Member

Triage: copy_file_range is now added for Linux/GNU but not for musl or Android. I couldn't find any reference that it's available on Android but I think it's fine to support musl skipping the test. Anyway, a rebase would be hard and it'd be better to open a new PR for it, so I'm going to close this PR. Thanks!

@JohnTitor JohnTitor closed this Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants