-
Notifications
You must be signed in to change notification settings - Fork 3
tools: source the "Permission denied" message from the OS (#159) #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
6ea1a48
15f8ecc
dbfec6e
92f639d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
|
|
||
| pub mod cli; | ||
| pub mod error; | ||
| pub mod os_error; | ||
| pub mod passwd; | ||
| pub mod validate; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // This file is part of the shadow-rs package. | ||
| // | ||
| // For the full copyright and license information, please view the LICENSE | ||
| // file that was distributed with this source code. | ||
|
|
||
| //! Error text sourced from the operating system instead of hardcoded. | ||
| //! | ||
| //! Wording for conditions that map to a libc `errno` is taken from | ||
| //! `strerror` (via [`std::io::Error`]) rather than carried as a string | ||
| //! literal in our tree. This keeps the text matching the host OS and lets | ||
| //! glibc translate it on localized systems — the same way GNU coreutils | ||
| //! renders system errors (e.g. `cat: /tmp: Is a directory`). See issue #159. | ||
|
|
||
| /// The operating system's message for a raw `errno`. | ||
| /// | ||
| /// For example `EACCES` renders as "Permission denied" on English locales | ||
| /// and the translated equivalent elsewhere. On targets whose libc does not | ||
| /// translate (musl), this is the untranslated English text. | ||
| #[must_use] | ||
| pub fn strerror(errno: i32) -> String { | ||
| // `io::Error::from_raw_os_error` carries libc's `strerror` text, but its | ||
| // `Display` appends Rust's own " (os error N)" suffix. `strip_errno` (the | ||
| // same helper uucore uses for its own I/O errors) removes it, leaving the | ||
| // bare OS message — matching GNU/coreutils output. | ||
| uucore::error::strip_errno(&std::io::Error::from_raw_os_error(errno)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think you should just remove strerror and call strip_errno everywhere directly a function with only one call isn't super interesting IMHO
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. Removing it.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — removed |
||
| } | ||
|
|
||
| /// The OS message for `EACCES` ("Permission denied"), sourced from libc. | ||
| #[must_use] | ||
| pub fn permission_denied() -> String { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am not sure we need this function either, just call the previous line
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to — one consideration: Do you still prefer inlining it everywhere, or would you rather a different shape — e.g. argument-less |
||
| strerror(libc::EACCES) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn permission_denied_is_nonempty_and_os_sourced() { | ||
| // We assert the shape, not the exact text: the wording comes from the | ||
| // host libc and may be localized, so hardcoding it would defeat the | ||
| // purpose of this module. | ||
| let msg = permission_denied(); | ||
| assert!(!msg.is_empty()); | ||
| // Same value libc would give for EACCES directly. | ||
| assert_eq!(msg, strerror(libc::EACCES)); | ||
| } | ||
|
pierre-warnier marked this conversation as resolved.
Outdated
|
||
|
|
||
| #[test] | ||
| fn strerror_drops_the_rust_os_error_suffix() { | ||
| // Regression guard: the bare OS message must not carry Rust's | ||
| // " (os error N)" rendering (the bug that made permission_denied() | ||
| // return "Permission denied (os error 13)"). | ||
| let msg = strerror(libc::EACCES); | ||
| assert!(!msg.contains("(os error"), "got: {msg:?}"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.rs/uucore/0.9.0/uucore/error/fn.strip_errno.html
we already have this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — switched to
uucore::error::strip_errno(it's already in 0.8, so no version bump). Addeduucoreto shadow-core's deps; it was already in the tree via the tool crates. Thanks!