Repository navigation
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| chownat(CWD, path.as_ref(), owner, group, flags).map_err(Into::into) | ||
| } | ||
|
|
||
| /// Redox does not provide rustix::fs::chownat. |
There was a problem hiding this comment.
add a TODO comment to remove once https://github.com/bytecodealliance/rustix/pull/1638 is released with rustix::fs::chownat
8c75df4 to
353b123
Compare
|
It seems |
| use tempfile::tempdir; | ||
|
|
||
| #[test] | ||
| fn test_chown_symlinks_and_unchanged_ids() { |
There was a problem hiding this comment.
THis PR is refactoring only. Would you remove this? We already have tests for real binaries.
There was a problem hiding this comment.
The PR changes an implementation, I think it is more than a refactoring.
There was a problem hiding this comment.
We should use tests/by-util/* instead in any cases.
353b123 to
410428a
Compare
|
Can we use rustix's chown (not at) at redox? |
| AtFlags::SYMLINK_NOFOLLOW | ||
| }; | ||
| // -1 is the chown(2) sentinel meaning that the corresponding ID is unchanged. | ||
| let owner = (uid != uid_t::MAX).then(|| Uid::from_raw(uid)); |
There was a problem hiding this comment.
do we need these two lines? Uid::from_raw(u32::MAX) is already -1 for the kernel
| #[cfg(target_os = "redox")] | ||
| fn chown<P: AsRef<Path>>(path: P, uid: uid_t, gid: gid_t, follow: bool) -> IOResult<()> { | ||
| let path = path.as_ref(); | ||
| let s = CString::new(path.as_os_str().as_bytes()).unwrap(); |
There was a problem hiding this comment.
the description says NUL paths now return an error, but this unwrap() is still there on redox, could you please map it to an error too?
410428a to
7d5168d
Compare
| fn chown<P: AsRef<Path>>(path: P, uid: uid_t, gid: gid_t, follow: bool) -> IOResult<()> { | ||
| let path = path.as_ref(); | ||
| if follow { | ||
| let owner = (uid != uid_t::MAX).then(|| rustix::fs::Uid::from_raw(uid)); |
There was a problem hiding this comment.
same owner/group mapping as in the other chown, could be a small shared helper, no?
Replace libc chown/lchown with rustix chownat on supported targets. Preserve symlink handling and unchanged-ID sentinels, and return an error for NUL-containing paths. Closes uutils#12041.
7d5168d to
d4c3e8e
Compare
Merging this PR will degrade performance by 15.86%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | true_consecutive_calls |
294.7 ns | 350.2 ns | -15.86% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing limadog9:fix/perms-use-rustix-chown-12041 (d4c3e8e) with main (6f60c70)2
Footnotes
-
70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(48f2a33) during the generation of this report, so 6f60c70 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
This branch has not been deployed
4 participants
Replace libc chown/lchown with rustix chownat on supported targets. Preserve symlink handling and unchanged-ID sentinels, and return an error for NUL-containing paths.
Closes #12041.