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.

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

Implement copy_file_range() #971

Closed
wants to merge 1 commit into from
Closed

Implement copy_file_range() #971

wants to merge 1 commit into from

Conversation

ArniDagur
Copy link
Contributor

This closes #941 (see issue for context).

I've decided that it is better for API forwards-compatibility to implement an empty CopyFileRangeFlags enum. I'm not sure how to do this in a good way, however, so please see the commit diff (48c9e6b) and advise me how to do this in a good way, if you'd like. I've also allowed edits from maintainers, so you can make a commit yourself.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This needs a CHANGELOG entry. Also, it needs a test and better documentation. Perhaps a doctest?

}
}

/// The flags argument is currently unused, and is provided to allow for future
Copy link
Member

Choose a reason for hiding this comment

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

How about documenting the rest of the function too, not just flags?

@ArniDagur
Copy link
Contributor Author

I've been struggling with the testing, so I'm wondering if you could lend a hand.

This code (using explicit file names /tmp/test.txt and /tmp/test2.txt for debugging purposes) works:

        let mut reader = File::open("/tmp/test.txt").unwrap();
        let mut writer = OpenOptions::new().read(true).write(true).truncate(true)
                            .open("/tmp/test2.txt").unwrap();

        let res = copy_file_range(reader.as_raw_fd(), None,
                                  writer.as_raw_fd(), None,
                                  2, CopyFileRangeFlags::empty())
                                  .unwrap();
        assert_eq!(2, res);

As seen here:

[arni@arni-pc][~/OpenSource/nix]% cat /tmp/test.txt
hello world
[arni@arni-pc][~/OpenSource/nix]% cat /tmp/test2.txt
he%      

But when trying to read the contents of /tmp/test2.txt after the copy, no bytes seem to be returned. I tried various methods.

@asomers
Copy link
Member

asomers commented Jan 4, 2019

I'm not very familiar with copy_file_range. Does it update the target's seek pointer? If so, that would explain your problem. Try seeking back to zero before reading test2.txt. Also, you'll need to rebase due to some changes to our CI setup.

@ArniDagur
Copy link
Contributor Author

Superseded by #1069.

@ArniDagur ArniDagur closed this May 26, 2019
bors bot added a commit that referenced this pull request Jun 16, 2019
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>
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.

Implement copy_file_range() system call for Linux
2 participants