Skip to content

Refactored docs for std::fs::set_permissions_nofollow + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag - #160170

Open
asder8215 wants to merge 4 commits into
rust-lang:mainfrom
asder8215:docs_set_perms_nofollow
Open

Refactored docs for std::fs::set_permissions_nofollow + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag#160170
asder8215 wants to merge 4 commits into
rust-lang:mainfrom
asder8215:docs_set_perms_nofollow

Conversation

@asder8215

@asder8215 asder8215 commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

View all comments

This PR refactors documentations for std::fs::set_permissions_nofollow and fixes BSD-based systems + Android to use fchmodat with AT_SYMLINK_NOFOLLOW flag (instead of no flag set) and refactors all other platforms to defer to OpenOptions with O_NOFOLLOW behavior.

r? @clarfonthey
Since they looked at the original set_permissions_nofollow PR I made

cc @RalfJung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 29, 2026
clarfonthey
clarfonthey reviewed Jul 29, 2026
View reviewed changes
Comment thread library/std/src/fs.rs Outdated
@clarfonthey

clarfonthey commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@rust-bors

rust-bors Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

📌 Commit d24bee2 has been approved by clarfonthey

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Jul 29, 2026
RalfJung
RalfJung reviewed Jul 29, 2026
View reviewed changes
Comment thread library/std/src/fs.rs Outdated
@RalfJung

RalfJung commented Jul 29, 2026

Copy link
Copy Markdown
Member

@bors r-

Sorry I have a question :)

@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2026
@rust-bors

rust-bors Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This pull request was unapproved.

View changes since this unapproval

@clarfonthey

clarfonthey commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

r? RalfJung since you're mostly reviewing already

@rustbot

rustbot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@asder8215
asder8215 force-pushed the docs_set_perms_nofollow branch from d24bee2 to 82ff1cb Compare July 29, 2026 21:45
@asder8215
asder8215 requested a review from RalfJung July 29, 2026 21:46
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 29, 2026
@asder8215 asder8215 changed the title Refactored docs for std::fs::set_permissions_nofollow clarifying BSD-based platforms behavior Refactored docs for std::fs::set_permissions_nofollow + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag Jul 29, 2026
@asder8215
asder8215 force-pushed the docs_set_perms_nofollow branch from 82ff1cb to f0403c2 Compare July 29, 2026 21:49
RalfJung
RalfJung reviewed Jul 30, 2026
View reviewed changes
Comment thread library/std/src/fs/tests.rs Outdated
RalfJung
RalfJung reviewed Jul 30, 2026
View reviewed changes
Comment thread library/std/src/fs/tests.rs Outdated
RalfJung
RalfJung reviewed Jul 30, 2026
View reviewed changes
Comment thread library/std/src/sys/fs/unix.rs Outdated
RalfJung
RalfJung reviewed Jul 30, 2026
View reviewed changes
Comment thread library/std/src/sys/fs/unix.rs Outdated
RalfJung
RalfJung reviewed Jul 30, 2026
View reviewed changes
Comment thread library/std/src/fs.rs Outdated
@RalfJung

RalfJung commented Jul 30, 2026

Copy link
Copy Markdown
Member

The user-facing comments are fine but this is turning into a libs discussion about the implementation.

r? libs

clarfonthey
clarfonthey reviewed Aug 23, 2026
View reviewed changes
Comment thread library/std/src/fs.rs Outdated
/// * On all other platforms, the behavior remains the same with
/// This function currently corresponds to the following underlying operations:
/// * Linux, BSD-based platforms, Android, QNX, NTO: `fchmodat` with `AT_SYMLINK_NOFOLLOW`.
/// If that is not supported, we fall back to `open` with `O_NOFOLLOW` followed by

@clarfonthey clarfonthey Aug 23, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// If that is not supported, we fall back to `open` with `O_NOFOLLOW` followed by
/// If that is not supported, we fall back to:

View changes since the review

@asder8215 asder8215 Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added the colon, but wasn't really sure the reason with deleting "open with O_NOFOLLOW followed by"

@clarfonthey clarfonthey Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looked redundant with the below bullet points to me:

/// If that is not supported, we fall back to: `open` with `O_NOFOLLOW` followed by
/// [`fs::set_permissions`].
/// * Other Unix-based platforms with symlinks: `open` with `O_NOFOLLOW` followed by
///   [`fs::set_permissions`].
/// * Other Unix-based platforms without symlinks: `open` followed by [`fs::set_permissions`].
/// * Windows: `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT` followed
///   by `SetFileInformationByHandle`.

clarfonthey
clarfonthey reviewed Aug 23, 2026
View reviewed changes
Comment thread library/std/src/sys/fs/unix.rs Outdated
clarfonthey
clarfonthey reviewed Aug 23, 2026
View reviewed changes
Comment thread library/std/src/sys/fs/unix.rs Outdated
clarfonthey
clarfonthey reviewed Aug 23, 2026
View reviewed changes
Comment thread library/std/src/sys/fs/unix.rs
@asder8215
asder8215 force-pushed the docs_set_perms_nofollow branch from 61a29b7 to b7b396a Compare August 24, 2026 01:42
@asder8215

asder8215 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the feedback (question on the doc feedback tho).
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 24, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@asder8215
asder8215 force-pushed the docs_set_perms_nofollow branch from b7b396a to 26c62dd Compare August 24, 2026 13:56
@clarfonthey

clarfonthey commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Looks good minus the nit on the doc comment. (r=me)

Also, non-blocking, but if you could replace the allow(unused) with allow(unused_mut), it would be slightly clearer. But it's not an issue otherwise.

@asder8215

asder8215 commented Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Also, non-blocking, but if you could replace the allow(unused) with allow(unused_mut), it would be slightly clearer. But it's not an issue otherwise.

I get different unused lint warnings depending on platform: for fchmodat supporting platforms it would be a unused_assignment warning, but for other platforms the warning on res would be unused_mut warning

Will update doc when I'm back on PC

@clarfonthey

clarfonthey commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

That makes sense; I'm fine just leaving it as that, then.

@clarfonthey

clarfonthey commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Just took a double-check at all the existing comments and resolved everything, since it looks like everything has been addressed. So, will merge this once you've made those updates. 👍🏻

@asder8215
Refactored set_perm_nofollow and have it written so that all supporte…
459be4d 
…d fchmodat platform call on fchmodat and every platform falls back to open + fchmod when _res is set to ErrorKind::Unsupported; updated docs to reflect change
@asder8215
asder8215 force-pushed the docs_set_perms_nofollow branch from 26c62dd to 459be4d Compare August 25, 2026 04:13
@clarfonthey

clarfonthey commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@bors r+ rollup

Thanks for working on this!

@rust-bors

rust-bors Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 459be4d has been approved by clarfonthey

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Aug 25, 2026
@clarfonthey

clarfonthey commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@bors note="Multi-platform code, has been tried but known to be unreliable" rollup=iffy

@rust-bors

rust-bors Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Unknown command "note". Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

@clarfonthey

clarfonthey commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@bors r+ note="Multi-platform code, has been tried but known to be unreliable" rollup=iffy

@rust-bors

rust-bors Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 459be4d has been approved by clarfonthey

It is now in the queue for this repository.

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Aug 25, 2026
@bors
Auto merge of #160170 - asder8215:docs_set_perms_nofollow, r=clarfonthey
9c04978 
Refactored docs for `std::fs::set_permissions_nofollow` + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag



This PR refactors documentations for `std::fs::set_permissions_nofollow` and fixes BSD-based systems + Android to use `fchmodat` with `AT_SYMLINK_NOFOLLOW` flag (instead of no flag set) and refactors all other platforms to defer to `OpenOptions` with `O_NOFOLLOW` behavior.

r? @clarfonthey
Since they looked at the original `set_permissions_nofollow` PR I made

cc @RalfJung
@rust-log-analyzer

rust-log-analyzer commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

The job arm-android failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
failures:

---- fs::tests::set_get_permissions_nofollows_symlink stdout ----

thread 'fs::tests::set_get_permissions_nofollows_symlink' (10644) panicked at library/std/src/fs/tests.rs:701:37:
called `Result::unwrap_err()` on an `Ok` value: ()
---- fs::tests::set_get_permissions_nofollows_symlink stdout end ----

failures:
    fs::tests::set_get_permissions_nofollows_symlink

test result: FAILED. 399 passed; 1 failed; 28 ignored; 0 measured; 0 filtered out; finished in 256.89s

@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2026
@rust-bors

rust-bors Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

💔 Test for 9c04978 failed: CI. Failed job:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@ChrisDenton ChrisDenton ChrisDenton left review comments
@clarfonthey clarfonthey clarfonthey left review comments
+2 more reviewers
@ivmarkov ivmarkov ivmarkov left review comments
@fs-rachel fs-rachel fs-rachel left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@clarfonthey clarfonthey

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.