Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adds implementation of copy_file_range() #1008

Closed
wants to merge 1 commit into from
Closed

Adds implementation of copy_file_range() #1008

wants to merge 1 commit into from

Conversation

edwinbdr
Copy link

@edwinbdr edwinbdr commented Jan 8, 2019

Also adds CHANGELOG.md and tests

@edwinbdr
Copy link
Author

edwinbdr commented Jan 8, 2019

This fixes #941

@asomers
Copy link
Member

asomers commented Jan 8, 2019

This doesn't even compile. Did you forget to commit something?

@edwinbdr
Copy link
Author

edwinbdr commented Jan 8, 2019

Forgot to commit something. Rebased the commit with the changes. Am a bit new to the multi-arch setup and accidently removed the import for the darwin target.

asomers
asomers reviewed Jan 8, 2019
View reviewed changes
src/fcntl.rs Outdated
/// On completion the number of bytes actually copied will be returned.
#[cfg(any(target_os = "android", target_os = "linux"))]
pub fn copy_file_range(
fd_in: RawFd, off_in: Option<&mut libc::loff_t>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the offsets provided as mutable references? If this operation changes them, could you describe how in the doc comment?

Also, it's ugly to expose raw libc types in our public API. In this case, I think it would be preferable for the arguments to be &mut i64.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree completely. I'll fix it to the &mut i64 and add some docs. Still wondering how far to stretch this. Wouldn't it be nicer to return a struct with the new state?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Returning a struct would be more Rusty than using mutable arguments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it would be a bit of an overkill and possibly also a memory management issue to return new structs. Purpose of this method is to quickly copy large parts of files incrementally, so it will most likely be called in an iterative fashion with feedback. Agree to leave it the way it is now, or do we want some extra helper methods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be ok to leave as is. Returning a structure with offsets could be up to 256 bits.

src/fcntl.rs
let off_out = off_out.map(|offset| offset as *mut _).unwrap_or(ptr::null_mut());

let ret = unsafe {
libc::syscall(libc::SYS_copy_file_range, fd_in, off_in, fd_out, off_out, len, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the last argument be flags instead of 0?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (flags removed)

test/test_fcntl.rs Outdated
tmp2.as_raw_fd(), None,
3, CopyFileRangeFlags::NONE).unwrap();

tmp2.sync_all().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I the sync_all necessary? I would think that the test should work fine without it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not. I was trying to be overly cautious.

src/fcntl.rs Outdated
fd_in: RawFd, off_in: Option<&mut libc::loff_t>,
fd_out: RawFd, off_out: Option<&mut libc::loff_t>,
len: usize,
_flags: CopyFileRangeFlags) -> Result<usize> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit I'm not enthusiastic about the flags argument. It currently provides no benefit to the user, and there's no guarantee that Linux will ever use it. But it does require a lot of typing. Maybe we should omit it. Or we could at least reduce the typing by changing the type to Into<CopyFileRangeFlags> so the user can simply supply 0. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux probably has it to provide forward compatibility. I'm ok with omitting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the flags.

@asomers
Copy link
Member

asomers commented Jan 14, 2019

The tests are failing because Travis's Linux kernel is too old. We could switch Linux testing from Travis to Cirrus, but I don't want to hold up this PR just for that. Why don't you just add an #[ignore] attribute for that test? Also, you'll need to rebase your commits to remove that merge commit from your history.

@edwinbdr
Adds implementation of copy_file_range()
44277be 
Also adds CHANGELOG.md and tests
asomers
asomers requested changes Jan 23, 2019
View reviewed changes
test/test_fcntl.rs
/// Linux version is too old for `copy_file_range`. Should work
/// on netbsd build.
#[test]
#[cfg_attr(target_env = "linux", ignore)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux is a target_os, not a target_env. Also, you should leave a message with the ignore attribute. And it's not going to work on a "netbsd build", because NetBSD doesn't have this syscall at all. Finally, we use Travis, not Jenkins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything else that is holding this PR back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The OP needs to address my last comments, fix the CI failures, and rebase.

@ArniDagur ArniDagur mentioned this pull request May 26, 2019
Merged
bors bot added a commit that referenced this pull request Jun 16, 2019
@bors @ArniDagur
Merge #1069
582846e 
1069: Implement copy_file_range() r=asomers a=ArniDagur

This should fix the problems with #971 and #1008.

Co-authored-by: Árni Dagur <arnidg@protonmail.ch>
@ArniDagur
Copy link
Contributor

This PR can be closed, right?

@asomers
Copy link
Member

asomers commented Jun 23, 2019

Yeah, since it was replaced by #1069 this PR can be closed.

@asomers asomers closed this Jun 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers requested changes

@ArniDagur ArniDagur ArniDagur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@edwinbdr @asomers @ArniDagur