Skip to content

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

Merged
andrewrk merged 1 commit intoziglang:masterfrom
LordMZTE:fchdir-cwd
Oct 21, 2023
Merged

std.os: do nothing when calling fchdir with AT_FDCWD#17616
andrewrk merged 1 commit intoziglang:masterfrom
LordMZTE:fchdir-cwd

Conversation

@LordMZTE
Copy link
Copy Markdown
Contributor

supersedes #15592
closes #9688

Comment thread lib/std/os.zig
} || UnexpectedError;

pub fn fchdir(dirfd: fd_t) FchdirError!void {
if (dirfd == AT.FDCWD) return;
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.

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

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

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

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

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.

@andrewrk andrewrk merged commit b3aaf85 into ziglang:master Oct 21, 2023
@andrewrk
Copy link
Copy Markdown
Member

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChildProcess.exec can't handle std.fs.cwd() as .cwd_dir

4 participants