minimal dirfd implementation (1/4) #146341

Qelxiros · 2025-09-08T17:32:07Z

This is the first of four smaller PRs that will eventually be equivalent to #139514.

A few notes:

I renamed new to open because open_dir takes &self and opens a subdirectory.
- I also renamed open to open_file.
I'm not sure how to impl AsRawFd and friends because the common implementation uses PathBufs. How should I proceed here?

The other PRs will be based on this one, so I'll make drafts and mark them ready as their predecessors get merged. They might take a bit though; I've never done this particular thing with git before.

Tracking issue: #120426

r? @tgross35

try-job: aarch64-apple
try-job: dist-various*
try-job: test-various*
try-job: x86_64-msvc-1
try-job: x86_64-mingw*

library/std/src/sys/fs/common.rs

library/std/src/sys/fs/uefi.rs

library/std/src/sys/fs/unsupported.rs

library/std/src/fs.rs

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

tgross35 · 2025-09-10T03:01:20Z

@ChrisDenton I'll need your help for the Windows review

library/std/src/sys/fs/unix.rs

Qelxiros · 2025-09-10T17:19:09Z

@rustbot ready

tgross35

Looks pretty good to me, just a few mechanical things here. There are a couple left over from the previous review, #146341 (comment), #146341 (comment), and (newly) #146341 (comment).

View changes since this review

library/std/src/sys/fs/unix/dir.rs

library/std/src/sys/fs/unsupported.rs

library/std/src/sys/fs/unix.rs

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

library/std/src/sys/fs/unix.rs

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

tgross35 · 2025-09-10T21:19:14Z

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

+    let mut handle = ptr::null_mut();
+    let mut io_status = c::IO_STATUS_BLOCK::PENDING;
+    let access = opts.get_access_mode()? | c::SYNCHRONIZE;
+    let options = create_options | c::FILE_SYNCHRONOUS_IO_NONALERT;


Could you add a note about why this flag is set?

I no longer remember why I chose this one, and looking at it now, I'm not sure whether we should set this one, FILE_SYNCHRONOUS_IO_ALERT, or neither. Maybe @ChrisDenton has thoughts?

Update, using neither causes an error, so I've added back FILE_SYNCHRONOUS_IO_NONALERT

tgross35 · 2025-09-10T21:24:27Z

From the top post:

* I'm not sure how to `impl AsRawFd` and friends because the `common` implementation uses `PathBuf`s. How should I proceed here?

I think it would be fine to include the impl AsRawFd only in unix/dir so it's only implemented if we have dirfd. I'll add an unresolved question to the tracking issue.

The other PRs will be based on this one, so I'll make drafts and mark them ready as their predecessors get merged. They might take a bit though; I've never done this particular thing with git before.

That is quite alright, there is no hurry :) For reference, the --update-refs rebase flag can be pretty useful when you're working with a stack (e.g. https://andrewlock.net/working-with-stacked-branches-in-git-is-easier-with-update-refs/)

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

rust-bors · 2025-09-11T04:22:54Z

💔 Test for ba4037e failed: CI. Failed jobs:

try - dist-various-2 (web logs, extended logs)

tgross35 · 2025-09-11T04:44:24Z

library/std/src/fs/tests.rs

@@ -1,5 +1,6 @@
 use rand::RngCore;

+use super::Dir;


This needs a gate on miri to not be unused

tgross35 · 2025-09-11T16:27:13Z

@bors2 try

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

rust-bors · 2025-09-11T17:07:31Z

💔 Test for 26d8112 failed: CI. Failed jobs:

try - dist-various-2 (web logs, extended logs)

tgross35 · 2025-09-11T18:56:52Z

@bors2 try

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

rust-bors · 2025-09-11T21:02:57Z

💔 Test for d36ef83 failed: CI. Failed jobs:

try - x86_64-msvc-1 (web logs, extended logs)

library/std/src/fs/tests.rs

tgross35 · 2025-09-15T20:21:12Z

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

+        if path.is_absolute() {
+            return File::open(path, opts);
+        }


Does nt_create_file not accept absolute paths if a root is specified? A comment about why this is needed would be helpful.

When the check is removed (and add the check shown in the error message), I get this error.

thread 'fs::tests::test_dir_read_file' (6364) panicked at library\std\src\fs\tests.rs:2251:17: dir.open_file(tmpdir.join("foo.txt")) failed with: The filename, directory name, or volume label syntax is incorrect. (os error 123)

Huh, does calling maybe_verbatim make a difference? Speculating, but maybe it doesn't like the Unix path join. Could you maybe debug dump the buffer from object_attributes.ObjectName in nt_create_file to see what it's trying to open?

and add the check shown in the error message

Which check is this?

dir.open_file(tmpdir.join("foo.txt"))

I'm not sure what you mean by unix path join. TempDir::join calls out to Path::join, which uses PathBuf::push, which appears to have lots of Windows-specific logic (although it's largely not cfg-gated, so I could be wrong).

I confirmed that the path it's attempting to open is consistent with tmpdir path + "foo.txt".

For reference (from https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile describing RootDirectory):
If this value is non-NULL, the ObjectName member specifies a file name relative to this directory.

I was thinking that .join() used a forward slash separator even on Windows - but I guess that's not accurate, looks like MAIN_SEP_STR is used. It would still be good to print it if you're able, to make sure it looks like a valid path, and to try to repro in C.

The error condition still seems weird - I don't see any mention of absolute paths at https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile. We should at least be able to explain what's going on before adding our own check.

Did you see my last message? I haven't reproduced in C, but I'm not sure I want to in a VM. The quote in my previous message seems ambiguous to me, so I'm not super surprised that it's a hard error.

Indeed I did not thanks. GH never seems to let me know if a new message appears while I'm typing.

Started some scratch work to test this out (not yet working for relative paths either though) https://godbolt.org/z/x81M1jdvf.

library/std/src/fs/tests.rs

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

tgross35 · 2025-09-15T20:25:58Z

@bors2 try

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc-1 try-job: x86_64-mingw*

rust-bors · 2025-09-15T23:24:01Z

☀️ Try build successful (CI)
Build commit: 6864db0 (6864db0e62c9a69c5b6b3a45634c65cedb96ff25, parent: f3fd3efe4f698ad9dc2ccd6b46a3b07e1bc911da)

tgross35 · 2025-09-19T07:03:54Z

library/std/src/fs/tests.rs

 use crate::test_helpers::{TempDir, tmpdir};
 use crate::time::{Duration, Instant, SystemTime};
-use crate::{env, str, thread};
+use crate::{env, io, str, thread};


io needs the cfg(not(miri)) gate as well

tgross35 · 2025-09-19T07:10:37Z

Status update: the current implementation looks pretty good to me, but I'd Chris or somebody more familiar with Windows to take a look at that bit.

It would still be good to add comments about the flags used to make the incantation a bit less magic without referring to the docs (e.g. FILE_SYNCHRONOUS_IO_NONALERT, FILE_FLAG_BACKUP_SEMANTICS).

tgross35 · 2025-09-24T04:51:29Z

@ChrisDenton would you be able to take a look at the windows impl here? Feel free to punt this bit to somebody else as well, just needs a second set of eyes since I'm not super familiar here.

Qelxiros · 2025-10-06T19:07:33Z

@ChrisDenton friendly ping

rustbot assigned tgross35 Sep 8, 2025

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 Sep 8, 2025

Qelxiros force-pushed the dirfd-minimum branch from c3278f6 to 6982a46 Compare September 8, 2025 17:37

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the dirfd-minimum branch from 6982a46 to 313c731 Compare September 8, 2025 18:26

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the dirfd-minimum branch from 313c731 to 1b3d7d6 Compare September 8, 2025 18:55

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the dirfd-minimum branch from 1b3d7d6 to be0d761 Compare September 9, 2025 15:09

tgross35 requested changes Sep 10, 2025

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2025

tgross35 requested a review from ChrisDenton September 10, 2025 03:01

Qelxiros force-pushed the dirfd-minimum branch from be0d761 to 3083d58 Compare September 10, 2025 17:17

Qelxiros commented Sep 10, 2025

View reviewed changes

library/std/src/sys/fs/unix.rs Outdated Show resolved Hide resolved

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 Sep 10, 2025

tgross35 reviewed Sep 10, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

rust-bors bot added a commit that referenced this pull request Sep 10, 2025

Auto merge of #146341 - Qelxiros:dirfd-minimum, r=<try>

0ac54d6

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

rust-bors bot added a commit that referenced this pull request Sep 10, 2025

Auto merge of #146341 - Qelxiros:dirfd-minimum, r=<try>

ba649e4

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

This comment has been minimized.

Sign in to view

tgross35 reviewed Sep 11, 2025

View reviewed changes

library/std/src/fs/tests.rs

@@ -1,5 +1,6 @@

use rand::RngCore;

use super::Dir;

Copy link

Contributor

tgross35 Sep 11, 2025

Choose a reason for hiding this comment

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

This needs a gate on miri to not be unused

Qelxiros force-pushed the dirfd-minimum branch from 20bd04e to 73435f8 Compare September 11, 2025 15:17

rust-bors bot added a commit that referenced this pull request Sep 11, 2025

Auto merge of #146341 - Qelxiros:dirfd-minimum, r=<try>

26d8112

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the dirfd-minimum branch 2 times, most recently from 1f8f886 to 109188b Compare September 11, 2025 17:15

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Sep 11, 2025

Auto merge of #146341 - Qelxiros:dirfd-minimum, r=<try>

d36ef83

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc* try-job: x86_64-mingw*

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the dirfd-minimum branch from 109188b to a37578d Compare September 15, 2025 16:43

Qelxiros requested a review from tgross35 September 15, 2025 18:46

tgross35 reviewed Sep 15, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Sep 15, 2025

Auto merge of #146341 - Qelxiros:dirfd-minimum, r=<try>

6864db0

minimal dirfd implementation (1/4) try-job: aarch64-apple try-job: dist-various* try-job: test-various* try-job: x86_64-msvc-1 try-job: x86_64-mingw*

Qelxiros force-pushed the dirfd-minimum branch from a37578d to cfbcd8b Compare September 15, 2025 21:36

tgross35 reviewed Sep 19, 2025

View reviewed changes

Add Dir::open(_file) and some trait impls

18a93b3

Qelxiros force-pushed the dirfd-minimum branch from cfbcd8b to 18a93b3 Compare September 19, 2025 15:29

minimal dirfd implementation (1/4) #146341

Are you sure you want to change the base?

minimal dirfd implementation (1/4) #146341

Conversation

Qelxiros commented Sep 8, 2025 • edited by tgross35 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgross35 commented Sep 10, 2025

Uh oh!

Uh oh!

Qelxiros commented Sep 10, 2025

Uh oh!

tgross35 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Sep 10, 2025

Uh oh!

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment has been minimized.

rust-bors bot commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Sep 11, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Sep 11, 2025

Uh oh!

tgross35 commented Sep 11, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Sep 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Qelxiros commented Sep 8, 2025 •

edited by tgross35

Loading

tgross35 left a comment •

edited

Loading