Repository navigation
cp: create removed destinations exclusively - #15330
abendrothj wants to merge 1 commit into
Conversation
Merging this PR will improve performance by 17.24%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | factor_three_39_bit_primes |
567 ms | 396.9 ms | +42.84% |
| ⚡ | Simulation | factor_thirteen_39_bit_primes |
9.5 s | 8.7 s | +8.87% |
| ⚡ | Simulation | factor_five_38_bit_primes |
1.6 s | 1.6 s | +3.62% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing abendrothj:cp-recreate-dest-exclusive (be468b3) with main (c76f272)
Footnotes
-
70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
| } | ||
| // Same exclusive create as for a data copy, which only does so on | ||
| // Unix (see `create_dest`). | ||
| let exclusive = cfg!(unix) && create_exclusive; |
There was a problem hiding this comment.
why only unix? create_new works on windows too
There was a problem hiding this comment.
Done for --attributes-only. On Windows the data copy goes through fs::copy, which can't create exclusively; that needs CopyFileExW with COPY_FILE_FAIL_IF_EXISTS. I can do that in a follow-up.
| translate!("cp-error-cannot-create-regular-file", "path" => dest.quote()), | ||
| ) | ||
| })?; | ||
| let mut dst_file = create_dest(dest, exclusive)?; |
There was a problem hiding this comment.
this also changes the initial mode of the stream dest on macOS (0o622 -> 0o600), please mention it in the commit message
| } | ||
| // Opening the writer releases cp's source open. Open it non-blocking, so a | ||
| // cp that never opens the source fails the test instead of hanging it. | ||
| let mut writer = loop { |
There was a problem hiding this comment.
this test is quite complex (polling loops, ENXIO, BrokenPipe), could it be simpler? i am worried it will be flaky on CI
There was a problem hiding this comment.
Simplified: one write to the FIFO after dest is replaced, so the ENXIO loop and the BrokenPipe check are gone. The wait for cp to remove dest stays, since the FIFO is what holds cp there, now with a 60s deadline.
On macOS, a streamed destination is now created at 0600, as on Linux, instead of 0622 minus the umask.
8270ad8 to
be468b3
Compare
Closes #15329
When the destination was missing, or cp had removed or renamed it (
--remove-destination,-f,--backup), cp created the new file withoutO_EXCL. Whatever appeared at that name in between was opened, and a symlink was followed.cp now creates the file exclusively in those cases and fails if anything is there, with GNU's messages. A destination that is overwritten in place is opened as before. The flag lives in uucore's
create_dest_restrictive, whereexclusivereplacesnofollow. Its onlynofollowcaller, mv's cross-device fallback, now creates exclusively too, as GNU mv does. mv can't be paused between its removal and the create, so that path has no test of its own; the uucore unit tests cover the exclusive create.--attributes-onlycreates exclusively on Windows too; the Windows data copy still usesfs::copy.util/check-safe-traversal.shacceptsO_EXCLas well asO_NOFOLLOWfor mv's create.Checked against GNU coreutils 9.12 for
--remove-destinationwith a symlink, a dangling symlink and a regular file created after the removal: messages and exit status match. Addedtest_cp_does_not_open_dest_recreated_after_removal, which fails without the change on macOS and Linux.