find: handle -fprintf write errors without panicking - #840
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in find’s -printf/-fprintf actions when output writes/flushes fail (e.g., /dev/full), switching to normal error reporting and a non-zero exit status instead of unwinding.
Changes:
- Propagate write/flush errors from the
Printfmatcher instead ofunwrap()panics, and report errors to stderr withfind:prefix. - Track the
-fprintfoutput path so error messages can include the destination file. - Add regression tests: a unit test with a failing writer and a Linux-only integration test using
/dev/full.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/find/matchers/printf.rs |
Remove panics on write/flush; report write errors; include output path for -fprintf. |
src/find/matchers/mod.rs |
Wire -fprintf to pass (File, PathBuf) into Printf. |
tests/test_find.rs |
Add Linux regression test ensuring /dev/full write errors are reported without panicking. |
Suppressed comments (1)
src/find/matchers/printf.rs:667
-fprintfwrite failures currently callmatcher_io.quit(), which aborts the entire traversal on the first non-BrokenPipe output error. That can prevent subsequent actions with side effects (e.g.-delete,-exec) from running, and is also inconsistent withPrinter, which sets exit status 1 but continues (seesrc/find/matchers/printer.rs:54-68). Consider only setting the exit code and letting the walk continue so other actions can still execute.
matcher_io.set_exit_code(1);
matcher_io.quit();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if error.kind() == std::io::ErrorKind::BrokenPipe { | ||
| matcher_io.quit(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I agree that regular write errors stopping later actions. GNU find continues evaluating them and returns status 1 afterward, so I’ll remove quit() from that path.
However, I prefer to keep using MatcherIO instead of std::process::exit(). Matchers are library code, and exiting directly could hide an earlier unrelated failure. Leaving the existing status unchanged also keeps normal pipelines successful.
a6490f1 to
810cf6b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #840 +/- ##
==========================================
+ Coverage 92.15% 92.20% +0.05%
==========================================
Files 35 35
Lines 7377 7417 +40
Branches 383 386 +3
==========================================
+ Hits 6798 6839 +41
+ Misses 438 437 -1
Partials 141 141 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Commit 810cf6b has test result changes: bfs testsuite: |
810cf6b to
d7352d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/find/matchers/printf.rs:650
BrokenPipehandling here only setsmatcher_io.quit(). Inprocess_dir, quitting breaks the main loop but still runsmatcher.finished_dir(...)andmatcher.finished(...), which can dispatch buffered-exec ... +commands and other side effects even though the output pipe is broken. This differs fromPrinter(which callsstd::process::exit(0)) and can lead to unexpected side effects after a broken pipe.
fn handle_output_error(&self, error: &std::io::Error, matcher_io: &mut MatcherIO) {
if error.kind() == std::io::ErrorKind::BrokenPipe {
matcher_io.quit();
return;
}
tests/test_find.rs:1166
- This integration test asserts the OS error text "No space left on device", which can be localized and vary across environments. To keep the test stable, force the command locale to
C(or assert on the numericos error 28portion instead).
let result = ucmd()
.args(&[".", "-maxdepth", "0", "-fprintf", "/dev/full", "%p\n"])
.fails();
result.no_stdout();
let stderr = result.stderr_str();
assert!(stderr.contains("find:"));
assert!(stderr.contains("/dev/full"));
assert!(stderr.contains("No space left on device"));
assert!(!stderr.contains("panicked"));
|
Commit d7352d8 has test result changes: GNU findutils testsuite: bfs testsuite: |
d7352d8 to
19a2865
Compare
Fixes #697.
-printf and -fprintf no longer panic when writing or flushing
output fails. The error is reported normally and find exits with
status 1. For -fprintf, the error also includes the output file.
Broken pipes still exit successfully.
Also added a unit test with a failing writer and a Linux regression test
using /dev/full.