fix(mkfifo): report actual OS error instead of hardcoded 'File exists'#12691
fix(mkfifo): report actual OS error instead of hardcoded 'File exists'#12691koopatroopa787 wants to merge 2 commits into
Conversation
mkfifo always reported "cannot create fifo 'PATH': File exists" on failure, regardless of the actual reason. For example, trying to create a fifo inside a non-existent directory produced a misleading "File exists" message instead of "No such file or directory" like GNU mkfifo. create_fifo() now returns the underlying io::Error (via rustix::io::Errno on most platforms, or errno on Apple targets) instead of discarding it as Result<(), ()>, and the cannot-create-fifo translation string now interpolates the real error message (normalized through UIoError to strip the '(os error N)' suffix, matching GNU's output format). Fixes uutils#12669
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes mkfifo error reporting so it surfaces the actual OS error (instead of always “File exists”), and adds a regression test to ensure missing parent directories report the correct failure reason.
Changes:
- Add a regression test for creating a FIFO in a non-existent directory (issue #12669).
- Propagate real
io::Errordetails fromcreate_fifointo the user-facing error message. - Update English and French locale strings to include the dynamic error text.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/by-util/test_mkfifo.rs | Adds a regression test asserting ENOENT is reported for missing parent directory |
| src/uu/mkfifo/src/mkfifo.rs | Changes FIFO creation to return io::Error and threads it into translated error output |
| src/uu/mkfifo/locales/en-US.ftl | Updates error message template to interpolate { $error } |
| src/uu/mkfifo/locales/fr-FR.ftl | Updates error message template to interpolate { $error } |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
GNU testsuite comparison: |
… UIoError
UIoError normalises errno values through Rust's io::ErrorKind, which
maps EEXIST to 'Already exists' and EACCES to 'Access denied' — neither
matches GNU coreutils output ('File exists' / 'Permission denied').
Replace the UIoError::from(e) call with a small os_err_msg() helper
that calls e.to_string() and strips the ' (os error N)' suffix that
std::io::Error's Display appends, leaving the raw OS strerror text that
GNU produces.
Also fix test_create_fifo_permission_denied: it was asserting 'File
exists' for a permission-denied scenario (directory without execute
bit). That test was only passing before because the old hardcoded
message happened to say 'File exists' regardless of the real errno.
The correct expected message for EACCES is 'Permission denied'.
|
Hey @sylvestre — thanks for flagging, fixed in the latest push (d6e875e). Two issues were happening: 1. EEXIST/EACCES showing wrong text — I was routing through UIoError to strip the '(os error N)' suffix, but UIoError also normalises errno through Rust's io::ErrorKind, mapping EEXIST → 'Already exists' and EACCES → 'Access denied' rather than the OS strerror strings GNU uses. Replaced with a small os_err_msg() helper that strips just the suffix from io::Error::to_string(), giving 'File exists' / 'Permission denied' / 'No such file or directory' to match GNU output. 2. test_create_fifo_permission_denied was silently wrong — it was asserting 'File exists' for a permission-denied scenario (directory without execute bit). That only passed before because the hardcoded message ignored the real errno entirely. The correct message for EACCES is 'Permission denied', which the test now asserts. The OpenBSD style/lint failures look like a runner-side issue (pkg_add can't resolve rust-1.94.1 packages on that VM) — unrelated to this change. |
What
mkfifoalways reported:on any failure, regardless of the actual cause. For example, trying to create a fifo inside a directory that doesn't exist produced the misleading "File exists" instead of GNU's "No such file or directory":
How
create_fifo()previously discarded the underlying error and returnedResult<(), ()>, so the caller had no way to know why the syscall failed and just used a hardcoded locale string.This PR:
create_fifo()to returnResult<(), io::Error>, propagating the real OS error (rustix::io::Errno->io::Erroron most platforms,io::Error::last_os_error()on Apple targets where the libc fallback is used).mkfifo-error-cannot-create-fifotranslation strings (en-US and fr-FR) to interpolate{ $error }instead of a hardcoded "File exists" / "Le fichier existe".UIoErrorso the message matches GNU's format (stripping the(os error N)suffix thatstd::io::Error'sDisplayadds).Existing tests that rely on the "File exists" message for actual EEXIST cases (
test_create_one_fifo_already_exists,test_create_fifo_permission_denied,test_mkfifo_permission_unchanged_when_failed) keep passing since EEXIST still maps to "File exists" — only now it's the real error rather than a hardcoded guess.Added
test_create_fifo_in_missing_directoryto cover the ENOENT case described in the issue.Closes #12669
Testing
cargo check -p uu_mkfifo(cross-checked againstx86_64-unknown-linux-gnusincemkfifois Unix-only)cargo fmt -p uu_mkfifo -- --checkcodespellclean on touched lines