Skip to content
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

Add chown functions to std::os::unix::fs to change the owner and group of files #88953

Merged
merged 2 commits into from
Sep 17, 2021

Conversation

joshtriplett
Copy link
Member

This is a straightforward wrapper that uses the existing helpers for C
string handling and errno handling.

Having this available is convenient for UNIX utility programs written in
Rust, and avoids having to call unsafe functions like libc::chown
directly and handle errors manually, in a program that may otherwise be
entirely safe code.

In addition, these functions provide a more Rustic interface by
accepting appropriate traits and using None rather than -1.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

…p of files

This is a straightforward wrapper that uses the existing helpers for C
string handling and errno handling.

Having this available is convenient for UNIX utility programs written in
Rust, and avoids having to call unsafe functions like `libc::chown`
directly and handle errors manually, in a program that may otherwise be
entirely safe code.

In addition, these functions provide a more Rustic interface by
accepting appropriate traits and using `None` rather than `-1`.
@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2021
@joshtriplett
Copy link
Member Author

r? @dtolnay for libs-api. (Will file tracking issue and update issue in stability attribute once approved.)

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Sep 15, 2021
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me.

The nix crate implements this using newtypes to prevent permuting the uid/gid arguments: https://docs.rs/nix/0.22.1/nix/unistd/fn.chown.html. But u32 seems all right to me.

@the8472
Copy link
Member

the8472 commented Sep 15, 2021

and avoids having to call unsafe functions like libc::chown

That's a poor justification considering that nix offers a safe wrapper. I'm not opposed to inclusion in the standard library but the reasoning for inclusion should be defensible.

Edit: Should have read the whole thread...

@dtolnay
Copy link
Member

dtolnay commented Sep 15, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2021

📌 Commit 862d89e has been approved by dtolnay

@bors
Copy link
Contributor

bors commented Sep 15, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2021
@joshtriplett
Copy link
Member Author

@the8472 I'm currently using that exact wrapper in my own code, along with other bits of nix.

I think #88989 would be the right place to continue the conversation about this (and about more general policies). I've already posted a comment there about where I think the boundary should be between std functionality and nix functionality.

Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 16, 2021
Add chown functions to std::os::unix::fs to change the owner and group of files

This is a straightforward wrapper that uses the existing helpers for C
string handling and errno handling.

Having this available is convenient for UNIX utility programs written in
Rust, and avoids having to call unsafe functions like `libc::chown`
directly and handle errors manually, in a program that may otherwise be
entirely safe code.

In addition, these functions provide a more Rustic interface by
accepting appropriate traits and using `None` rather than `-1`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
…laumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#86422 (Emit clearer diagnostics for parens around `for` loop heads)
 - rust-lang#87460 (Point to closure when emitting 'cannot move out' for captured variable)
 - rust-lang#87566 (Recover invalid assoc type bounds using `==`)
 - rust-lang#88666 (Improve build command for compiler docs)
 - rust-lang#88899 (Do not issue E0071 if a type error has already been reported)
 - rust-lang#88949 (Fix handling of `hir::GenericArg::Infer` in `wrong_number_of_generic_args.rs`)
 - rust-lang#88953 (Add chown functions to std::os::unix::fs to change the owner and group of files)
 - rust-lang#88954 (Allow `panic!("{}", computed_str)` in const fn.)
 - rust-lang#88964 (Add rustdoc version into the help popup)
 - rust-lang#89012 (Suggest removing `#![feature]` for library features that have been stabilized)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 723d279 into rust-lang:master Sep 17, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 17, 2021
@joshtriplett joshtriplett deleted the chown branch September 19, 2021 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants