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

Tracking Issue for #![feature(unix_chown)] #88989

Closed
4 tasks done
joshtriplett opened this issue Sep 15, 2021 · 12 comments · Fixed by #113033
Closed
4 tasks done

Tracking Issue for #![feature(unix_chown)] #88989

joshtriplett opened this issue Sep 15, 2021 · 12 comments · Fixed by #113033
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Sep 15, 2021

Feature gate: #![feature(unix_chown)]

This is a tracking issue for the chown family of functions. These functions change the UNIX owner and group of a file or file descriptor.

Public API

pub fn chown<P: AsRef<Path>>(dir: P, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {}
pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {}
pub fn lchown<P: AsRef<Path>>(dir: P, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {}

Steps / History

@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Sep 15, 2021
@dtolnay
Copy link
Member

dtolnay commented Sep 15, 2021

I think as part of stabilizing, we'll want to chat about what the end game is here. Does std::os::unix take on much more of nix::unistd or nix more broadly, maybe with less type safety if we aren't interested in defining vocabulary types for Uid/Gid? Or only the most widely used APIs? By what metric?

cc @the8472

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 15, 2021

@dtolnay For my part, I'm not necessarily looking to take on all of nix's functionality. My metric is, roughly, "is this something that's trivial to do with a common shell command, and likely to be desirable in minimal standalone programs". Rust works really well for tiny static binaries, chroots, containers, inits, and similar; it's also helpful for things that need to run with privileges. I'm adding things that I'd like to use for such purposes.

So, for instance, I wouldn't want to add something at the level of quotactl or prctl or finit_module; those make sense to have in separate crates. But I'd like to add other things as fundamental as chroot and chown, such as pipe (now that we have OwnedFd), setgroups, and a setuid/setgid mechanism (if we can portably handle some of the mess there, since unfortunately not all systems have setresuid and setresgid).

One other component of this policy: I think we should only add things that represent well-established interfaces that won't need to change in the future. POSIX functions are fairly safe in that regard, as are some long-established and widely used target-specific functions, but we shouldn't be exposing the forefront of target-specific functionality.

@nagisa
Copy link
Member

nagisa commented Sep 15, 2021

Just a drive-by comment, but names for these functions seem to not follow the broader conventions used within the standard library (e.g. parent_id and not getppid), though I guess I'm seeing a fair number of other functions that don't refrain from using the original abbreviated names either (chroot?)

@the8472
Copy link
Member

the8472 commented Sep 15, 2021

Do we need them on the standard library though when alternatives exist? For example the pidfd extensions to Command make sense because one must opt-in at process creation time. Many of the methods in unistd don't need any supporting plumbing. So they seem redundant.

My metric is, roughly, "is this something that's trivial to do with a common shell command, and likely to be desirable in minimal standalone programs". Rust works really well for tiny static binaries, chroots, containers, inits, and similar

This is still nebulous. Shell scripts will often glue things together you can't find in the rust standard library. Even something as simple as decompressing a stream gzip (included in busybox or the python and java standard libraries) will require an external library. Ditto regular expressions. So "minimal" sounds more like a toy program if you eschew external libraries. Or at least a very peculiar, tiny scope that just happens to almost exactly match what the standard library currently offers.
And if the user does use libraries then we're back to the question what value std integration can provide compared to nix.

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 15, 2021

@nagisa I agree, and under other circumstances, we'd probably call it something a little less abbreviated like change_owner. Here's my justification for why I think chown makes more sense than change_owner or similar here:

If we're adding a function that's likely to run on any target supporting std, such as create_dir or rename, we're likely calling different functions on different targets. It makes sense to use a descriptive name, rather than a target-specific name for one target. And if we could make some kind of portable function to handle ownership changes, I would absolutely favor calling it by a general name.

However, the user and ownership model is so incredibly different between Windows and UNIX that I don't think such an abstraction layer makes sense in the standard library. I think we should build abstraction layers when platforms work more-or-less the same way ("make a directory"), not when we'd have to introduce a fair amount of opinionated policy to have any hope of a useful abstraction.

Given that, I think it makes sense to have a UNIX-specific function exposing the UNIX chown functionality here. And since it's UNIX-specific, and the functions to do this are portably called chown on all UNIX systems, and widely known by that name including via the command-line tool of the same name, to the point that it's a standard vocabulary verb among administrators, I think a more verbose name would be an obfuscation, not an improvement.

I think that justification holds for chroot, and chown. It may hold for some other things. There's other functionality, though, where I think we should improve on naming; for instance, I'm glad we have is_char_device rather than ifchr.

Does that seem reasonable, regarding naming?

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 15, 2021

Do we need them on the standard library though when alternatives exist? For example the pidfd extensions to Command make sense because one must opt-in at process creation time. Many of the methods in unistd don't need any supporting plumbing. So they seem redundant.

The same argument applies to many things in the standard library. std::os::unix::fs::symlink is fairly standalone; FileExt (read_at, write_at) could easily be done standalone. std::process::id could be standalone. Many pieces of the standard library aren't fundamentally things that have to be in the standard library; they could be in crates.

I'm in favor of caution; we don't want to add interfaces that we may regret later, because we can't remove them. But if we're confident that the interface will stand the test of time without needing to change, then I do think there's value in having a std interface.

@sunfishcode
Copy link
Member

The current definition of fchown takes its AsFd argument by value instead of by reference. This means that passing it a &File gets an error, and passing it a File consumes and closes the file. To be usable, fchown's argument should be changed from fd: F to fd: &F.

Alternatively, #93869 proposes to change AsFd so that functions should take AsFd arguments by value.

@joshtriplett
Copy link
Member Author

Good catch. I've marked that as blocking in the top comment; we shouldn't stabilize fchown until we select and apply an appropriate fix for that.

Further discussion in #93869.

@joshtriplett
Copy link
Member Author

I think all the issues with this have been resolved, and I'd like to propose stabilizing it.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 30, 2023

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 30, 2023
@joshtriplett joshtriplett added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Apr 29, 2023
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 14, 2023
@rfcbot
Copy link

rfcbot commented May 14, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 14, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 24, 2023
@rfcbot
Copy link

rfcbot commented May 24, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@Amanieu Amanieu removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label May 30, 2023
@bors bors closed this as completed in e0922fb Jul 22, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jul 23, 2023
Stabilize chown functions (`unix_chown`)

Closes #88989
FCP is complete here: rust-lang/rust#88989 (comment)
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Oct 17, 2023
Stabilize chown functions (`unix_chown`)

Closes #88989
FCP is complete here: rust-lang/rust#88989 (comment)
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Stabilize chown functions (`unix_chown`)

Closes #88989
FCP is complete here: rust-lang/rust#88989 (comment)
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Stabilize chown functions (`unix_chown`)

Closes #88989
FCP is complete here: rust-lang/rust#88989 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants