cp: take the standard copy to any non-regular destination - #14289
cp: take the standard copy to any non-regular destination#14289arbelonson-source wants to merge 3 commits into
Conversation
Closes uutils#14283. The sparse paths call `ftruncate` and write at explicit offsets, which only a regular file supports. Only a fifo destination was excluded, so a character device still took them and failed: $ cp --sparse=always file /dev/stdout > /dev/null cp: 'file' -> '/dev/stdout': Invalid argument GNU copies the contents normally. Widen the check from "is a fifo" to "is not a regular file", which covers character devices, sockets and block devices as well. A destination that does not exist yet is about to be created as a regular file, so it is still eligible.
9365e91 to
9009c5d
Compare
|
GNU testsuite comparison: |
|
Some dynamically gerated files on FUSE fs is not truncatable while it is still regular file. |
@oech3: "Please never check file type. Fallback when ftruncate failed to remove unnecessary overhead." / "Also `not` in the name of fn should be avoided and bool should be flipped even you want to keep the hack." `dest_cannot_be_sparse` cost every destination -- overwhelmingly a regular file, for which it always returned false -- a `metadata` call before any copy started, just to rule out the rare fifo/socket/character- device case. Removed, along with the `dest: &Path` parameter it needed in all four planning functions that called it. In its place, the two sparse-copy functions now catch their own first `ftruncate` failing with EINVAL and fall back to `buf_copy::copy_fast` right there. Nothing has been read from the source or written to the destination yet at that point, so falling back there cannot duplicate or corrupt output -- the same guarantee the old pre-check gave, discovered instead of assumed. The negated boolean predicate this replaces is gone rather than renamed, since a fallible action controlling what happens next reads better here than a bool a caller re-interprets afterward.
fall_back_if_cannot_be_sparse was only ever called from sparse_copy_without_hole_fd and sparse_copy_fd, each already inside a match/if on ftruncate's result -- pull it into a match at each call site instead of a third, indirected function. AI-assisted-by: Claude Opus 5, via Claude Code
|
Good point to check — I built a local test to verify rather than assume. Wrote a minimal FUSE filesystem (Python/fusepy) exposing a file that reports as a regular file via `stat` (`S_IFREG`, size 45) but rejects `truncate`/`write` with `ENOSYS`, mimicking a dynamically-generated FUSE file: Byte-identical error and exit code (1) on both. GNU doesn't gracefully copy to this kind of destination either — it fails the same way, and the failure happens at file-open time (before the `ftruncate` fallback this PR adds is ever reached), so the fallback doesn't change behavior here. The case this PR actually widens the fallback for is a destination that opens fine but rejects `ftruncate` specifically with `EINVAL` (fifo, socket, char/block device) — this FUSE case fails earlier than that and isn't affected either way. If you have a specific FUSE filesystem in mind that behaves differently from what I simulated (e.g., one where open/write succeed but only truncate fails with something other than what leads to this same open-time error), let me know and I'll test that shape specifically — happy to be wrong here, I just don't have a concrete repro to test yet beyond this. |
|
@arbelonson-source please read the AI policy |
| fn test_cp_sparse_always_to_character_device() { | ||
| // The sparse paths use ftruncate and positional writes, which a character | ||
| // device rejects with EINVAL. Only a fifo destination was excluded, so | ||
| // `cp --sparse=always FILE /dev/null` failed where GNU copies normally. |
There was a problem hiding this comment.
| fn test_cp_sparse_always_to_character_device() { | |
| // The sparse paths use ftruncate and positional writes, which a character | |
| // device rejects with EINVAL. Only a fifo destination was excluded, so | |
| // `cp --sparse=always FILE /dev/null` failed where GNU copies normally. | |
| fn test_cp_sparse_always_to_non_truncatable() { | |
| // The sparse paths use ftruncate. Fallback to normal copy when target is not truncatable e.g. `/dev/null`. |
More generic name.
| let mut src = src_file; | ||
| let mut dst = dst_file; |
There was a problem hiding this comment.
Do we really need to rename them?
3 participants
Closes #14283.
GNU copies the contents normally.
Why
The sparse paths call
ftruncateand then write at explicit offsets, and only a regular file supports either. A character device rejectsftruncatewithEINVAL, which is theInvalid argumentabove.handle_reflink_never_sparse_alwaysalready guards against this, but only for a fifo:A fifo is not the only destination that cannot be written sparsely. This widens the check to "is not a regular file", which covers character devices, sockets and block devices too. A destination that does not exist yet is still eligible, since it is about to be created as a regular file.
Worth noting why the report is easy to miss:
/dev/stdoutresolves to whatever stdout is attached to. Piped, it is a fifo and the old check caught it; redirected to a file, it is a regular file and the sparse path works. It only fails when stdout is a character device — a terminal, or> /dev/null— which is what the reporter had.Testing
All four
--sparsemodes now match GNU coreutils 9.11 to a character device, and sparseness is still preserved where it matters:test_cp_sparse_always_to_character_device, verified to fail onmainand pass with the changecargo test --features cp --no-default-features: 368 passed, 0 failed (367 pre-existing, 1 new)cargo fmt --checkandcargo clippy -p uu_cp --all-targets: cleanFileTypeExtimport is droppedDisclosure
Prepared with AI assistance (Claude Opus 5, via Claude Code), per the AI policy in CONTRIBUTING.md. The GNU behaviour above was established by running the installed GNU binary as a black box; I did not read GNU coreutils source. All testing was run locally.