Skip to content

std.os: do nothing when calling fchdir with AT_FDCWD #17616

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.

Sign up for GitHub

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

Merged
andrewrk merged 1 commit into from
Oct 21, 2023
Merged

std.os: do nothing when calling fchdir with AT_FDCWD #17616

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/std/os.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2966,6 +2966,7 @@ pub const FchdirError = error{
} || UnexpectedError;

pub fn fchdir(dirfd: fd_t) FchdirError!void {
if (dirfd == AT.FDCWD) return;
Copy link
Copy Markdown
Contributor

@rootbeer rootbeer Oct 19, 2023

Choose a reason for hiding this comment

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

Seems odd for Zig to change the semantics of fchdir() in this case. The underlying syscall returns EBADF, because it is a bad file descriptor. AT_FDCWD is defined for the openat() call, and probably shouldn't be a special case anywhere else?

Also, if you want Zig's fchdir() to understand AT_FDCWD, then it seems like it should check the directory to see if any of the error cases (e.g. Access Denied, or NotDir) apply.

Copy link
Copy Markdown
Member

@squeek502 squeek502 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Some context:

  • std.fs.cwd() returns a Dir with fd AT_FDCWD, which is not a real handle (it's the constant -100)
  • This particular change is meant to enable calling std.ChildProcess.exec with .cwd_dir = std.fs.cwd()

So, either the strategy for std.fs.cwd() needs to change (would need to make it return a real fd) or we need some special handling of AT_FDCWD throughout the standard library. I'm not totally sure what the best way to go in the long run will be, but this particular change doesn't seem like it has many/any downsides, as I'm unsure why it would make sense for fchdir with AT_FDCWD to return an error--it's almost by definition a no-op.

More discussion in #9688

Copy link
Copy Markdown
Contributor

@rootbeer rootbeer Oct 20, 2023

Choose a reason for hiding this comment

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

Ah, the use of AT_FDCWD by std.fs.cwd() seems a bit funky to me. AFAICT, in Linux the -100 is only magical to the openat call (its the source of the "AT_" prefix on the symbol, I believe).

If Zig wants to support a fake file descriptor number to represent the "current working directory" without actually opening it, then it should probably be a constant not defined by openat? This does seem likely to lead to some awkward special casing, like should getdents and fstat support this special constant too? (Is std.fs.stat(std.fs.cwd()) is a thing too?)

Also, I'm not sure where the code involved here falls on the stick-to-Posix versus Zig-universal-library discussion (fchdir is strictly Posix-land?, and ChildProcess is Zig?)

Actually .. is a bigger problem that the lazily resolved -100 from stat.fs.cwd() represents the wrong directory if its followed by a chdir() before being used? E.g., if I create a ChildProcess with .cwd_dir = std.fs.cwd(), then do a chdir() to somewhere else, then start the child process, which "current" directory should the child get?

Copy link
Copy Markdown
Member

@squeek502 squeek502 Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

should getdents and fstat support this special constant too?

This has yet to be figured out, but if Zig wants to stick with std.fs.cwd() returning AT_FDCWD, then it'll need some handling at some level of abstraction in all the places it can cause problems (you're right that std.fs.cwd().stat() will hit the same problem in fstat). This is why I'm unsure if this is a good/viable long term plan, but right now std.fs.cwd() really relies on being essentially free (i.e. it can't fail and it can be called all over the place without opening a new fd each time).

Copy link
Copy Markdown
Contributor Author

@LordMZTE LordMZTE Oct 20, 2023

Choose a reason for hiding this comment

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

I believe that it might be a good idea to handle AT_FDCWD in other functions in std.os. However, this PR has been implemented as requested by Andrew in the PR I've linked in this one's description.

while (true) {
switch (errno(system.fchdir(dirfd))) {
.SUCCESS => return,
Expand Down