cp: implement sparse copy support for windows#12686
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
GNU testsuite comparison: |
|
@oech3 Ready for review. Failing check is unrelated to these changes. |
|
please add or enable relevant tests, thanks |
There are three new tests for this feature on windows, since this is windows specific PR. Is there anything else I could have done that I missed? |
|
@sylvestre bump |
1 similar comment
|
@sylvestre bump |
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
Barring a few issues, this looks like a great addition. However, I find myself wishing we could use a safe API of the windows-*/winapi_util crates, instead of raw win32 unsafe calls. If it's not available there, would you add safe wrappers of these to uucore::fs and use them here?
Also, can someone more familiar with Windows I/O take a second look? I can only comment on my limited experience here.
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
Super nice! This has definitely improved a lot since I last took a lot at it; I especially like the safe wrappers around winapi. I don't have much to comment, but I have spotted a few issues that are worth taking a look at.
| copy_debug.sparse_detection = SparseDebug::Zeros; | ||
| sparse_copy(&mut src_file, dest).map_err(context_err)?; | ||
| } else { | ||
| std::fs::copy(source, dest).map_err(context_err)?; |
There was a problem hiding this comment.
Can't we just reuse the open handle and do std::io::copy(&src_file, dest)? This especially concerns me due to possible TOCTOUs.
There was a problem hiding this comment.
Eh, here we get to decide which way to go. If we keep fs::copy(CopyFileExW) we get kernel copy path, auto ReFS block cloning and SMB offload on hotpaths. And while there could be a race we never get incorrect content. We would give up all of that if we go with io::copy. I'm for fs::copy but if the pros don't matter I'll revert to io::copy.
There was a problem hiding this comment.
hmm it's true there's no really room for harm, as with the other one. It can be kept as-is then, but please document it as intentional :)
EDIT: it seems it was already, but GitHub decided not to update the diff lol. Okay, this is an A-OK from me.
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
A-OK. High quality code that enables missing functionality on Windows.
4 participants
Attempts to fix: #12186