Skip to content

dirfd file operations (2/4)#150679

Open
Qelxiros wants to merge 2 commits intorust-lang:mainfrom
Qelxiros:dirfd-files
Open

dirfd file operations (2/4)#150679
Qelxiros wants to merge 2 commits intorust-lang:mainfrom
Qelxiros:dirfd-files

Conversation

@Qelxiros
Copy link
Copy Markdown
Contributor

@Qelxiros Qelxiros commented Jan 4, 2026 •
edited by rustbot
Loading

Previous PR: #146341
Reference: #139514
Tracking issue: #120426

@rustbot rustbot added O-windows Operating system: Windows 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 Jan 4, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jan 4, 2026

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Qelxiros
Copy link
Copy Markdown
Contributor Author

Qelxiros commented Jan 4, 2026

This is going to need a try run for windows at least. @tgross35 I'll let you decide which jobs make the most sense here.

@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Jan 4, 2026

@bors2 try jobs=x86_64-msvc-*

Cc @the8472 for dirfd, and @ChrisDenton for Windows

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot added a commit that referenced this pull request Jan 4, 2026
@rust-bors
Auto merge of #150679 - Qelxiros:dirfd-files, r=<try>
796e04e 
dirfd file operations (2/4)

try-job: x86_64-msvc-*
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jan 5, 2026

☀️ Try build successful (CI)
Build commit: 796e04e (796e04e9c306cfa50aa215b1e93ebdb1cbdc51d8, parent: e29fcf45e4ae686d77b490bf07320f0d3a2cf35f)

@rust-bors

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Jan 24, 2026

I haven't had a chance to look here yet, sorry

@rustbot reroll

@rustbot rustbot assigned ChrisDenton and unassigned tgross35 Jan 24, 2026
@rustbot

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@Qelxiros Qelxiros mentioned this pull request Mar 30, 2026
Draft
@Qelxiros
Copy link
Copy Markdown
Contributor Author

Qelxiros commented Apr 1, 2026

@rustbot reroll

Mark-Simulacrum
Mark-Simulacrum reviewed Apr 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

The Windows code looks like it could use a second set of eyes from someone with Windows knowledge, unless we manage to remove most of the new code through reusing the existing rename logic.

View changes since this review

Comment thread library/std/src/sys/fs/windows/dir.rs Outdated
opts.access_mode(c::DELETE);
opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT | c::FILE_FLAG_BACKUP_SEMANTICS);
let handle = self.open_file_native(from, &opts, dir)?;
// Calculate the layout of the `FILE_RENAME_INFORMATION` we pass to `NtSetInformationFile`
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum Apr 4, 2026

Choose a reason for hiding this comment

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

A bunch of the content here looks similar to

rust/library/std/src/sys/fs/windows.rs

Line 1313 in 2972b5e

let err = api::get_last_error();
-- could we reuse some of that? Is there an opportunity to use MoveFileExW for this path at all?

Copy link
Copy Markdown
Contributor Author

@Qelxiros Qelxiros Apr 30, 2026

Choose a reason for hiding this comment

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

MoveFileExW operates exclusively on filenames. Dir::rename is intended to be TOCTOU-safe, so it needs to use directory Handles. I've factored out the NtSetInformationFile section of both functions into nt_rename. The implementations are a little different from one another, so I'm not 100% sure I got the function signature right. I'll wait for the CI to run and reevaluate.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2026
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot added the O-windows Operating system: Windows label Apr 30, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 30, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Qelxiros
Copy link
Copy Markdown
Contributor Author

Qelxiros commented Apr 30, 2026

@rustbot label -O-windows
(pending CI) @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2026
@Qelxiros
Copy link
Copy Markdown
Contributor Author

Qelxiros commented Apr 30, 2026

Oh, we probably also want a try jobs=x86_64-msvc-* as above.

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

Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

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

Assignees

@Mark-Simulacrum Mark-Simulacrum

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.

5 participants

@Qelxiros @rustbot @tgross35 @Mark-Simulacrum @ChrisDenton