mkdir:Refactor: replace direct libc calls with nix wrappers for umask/mkdirat#11138
mkdir:Refactor: replace direct libc calls with nix wrappers for umask/mkdirat#11138mattsu2020 wants to merge 1 commit intouutils:mainfrom
libc calls with nix wrappers for umask/mkdirat#11138Conversation
|
GNU testsuite comparison: |
|
|
||
| use clap::builder::ValueParser; | ||
| use clap::parser::ValuesRef; | ||
| use clap::{Arg, ArgAction, ArgMatches, Command}; |
There was a problem hiding this comment.
Why are you putting this in this PR???
There was a problem hiding this comment.
These imports are required for this refactor: UmaskGuard now uses nix::sys::stat::umask/Mode, and create_dir_with_mode uses DirBuilderExt::mode.
There was a problem hiding this comment.
Each of these imports is only used oncetwice, I think we can remove them and qualify the method calls instead for clarity.
There was a problem hiding this comment.
Personally if it's less than three, I would just qualify them
|
@mattsu2020 The change is sound, but the commit description isn't helpful. In general, replacing unsafe |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing Footnotes
|
|
@mattsu2020 Could you please run |
4829bab to
0840bb7
Compare
|
GNU testsuite comparison: |
src/uu/mkdir/src/mkdir.rs
Outdated
| use nix::sys::stat::{Mode, umask}; | ||
| use std::ffi::OsString; | ||
| use std::io::{Write, stdout}; | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
this import can go back in create_dir_with_mode?
There was a problem hiding this comment.
Thanks, good point. I’ll move DirBuilderExt back into create_dir_with_mode. Mode/umask will stay at module scope because UmaskGuard also uses them.
|
GNU testsuite comparison: |
src/uu/mkdir/src/mkdir.rs
Outdated
| let old_mask = unsafe { uucore::libc::umask(new_mask) }; | ||
| Self(old_mask) | ||
| /// Set umask to 0 and return a guard that restores the original on drop. | ||
| fn set_zero() -> Self { |
There was a problem hiding this comment.
can rename this function to new, see existing pattern in:
coreutils/tests/by-util/test_env.rs
Lines 974 to 984 in 6100aee
|
@mattsu2020 do you intend to continue with this PR? |
Replace direct `libc` syscall usage with `nix` wrapper to reduce unsafe
surface area. The `UmaskGuard` now uses `nix::sys::stat::umask` and
`Mode` instead of `uucore::libc::umask`.
- Use `nix::sys::stat::{Mode, umask}` instead of `unsafe { libc::umask() }`
- Rename `UmaskGuard::set_zero()` to `UmaskGuard::new()` for consistency
with existing patterns (e.g., SigpipeGuard in test_env.rs)
- Move `DirBuilderExt` import inside `create_dir_with_mode` function
Summary
This PR replaces a few direct
libcsyscall usages withnixwrappers to reduce unsafe surface area and improve consistency in Unix-specific filesystem/process code.