Heightenize - #12681
Conversation
| res = reader_read(parser, libc::STDIN_FILENO, &IoChain::new()); | ||
| } else { | ||
| let n = wcs2bytes(&args[my_optind]); | ||
| let filename = &args[my_optind]; |
There was a problem hiding this comment.
wouldn't hurt to put unrelated changes like this and the cwd_fd if-let cleanup in separate commits.
There was a problem hiding this comment.
Sorry, this one is a leftover from a different attempt where I had replaced File::open with wopen_cloexec to have an implicit heightenization. Then I changed my mind and reverted back to File::open and didn't realize this made this filename stuff unnecessary.
And for the cwd_fd cleanup, it didn't feel worthwhile to have a separate commit for it, especially since you usually ask me to squash commits instead😊
There was a problem hiding this comment.
I had replaced File::open
I was gonna comment that we should probably extract a version of File::open that heightenizes implicitly.
So yeah, something like that would make sense.
And for the cwd_fd cleanup, it didn't feel worthwhile to have a separate commit for it, especially since you usually ask me to squash commits instead😊
oh. if it's unrelated changes then separating is usually good
| /// Return the fd, which always has CLOEXEC set; or an invalid fd on failure, in | ||
| /// which case an error will have been printed, and the input fd closed. | ||
| fn heightenize_fd(fd: OwnedFd, input_has_cloexec: bool) -> nix::Result<OwnedFd> { | ||
| pub fn heightenize_fd(fd: OwnedFd, input_has_cloexec: bool) -> nix::Result<OwnedFd> { |
There was a problem hiding this comment.
lsof -p $fish_pid shows that we also have some long-lived fds in inotify/kqueue/notifyd code.
I'll add fixes for those.
Except for notifyd because as I understand https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/notify_register_file_descriptor.3.html,
notify_cancel closes the file descriptor we obtained from notify_register_file_descriptor (even though LLM disagrees),
so I'm not sure if we can safely dup and close that one.
Grepping for File::open also shows fish_indent, which may be a builtin, so it shares fds with fish.
I guess that one we can count as short-lived enough for now because there's no concurrent execution of builtins yet.
There was a problem hiding this comment.
Yes, I expect they are other cases that need to be solved. I didn't do a deep dive. I only fixed the one I could see by just running fish. And by doing it fish_open, I figured it would fix a few I wasn't seeing. And I only dived beyond that just enough to get a couple tests.
|
|
||
| /// A file descriptor holding the current working directory, for use in openat(). | ||
| /// This is never null and never invalid. | ||
| pub cwd_fd: Option<Arc<OwnedFd>>, |
There was a problem hiding this comment.
yeah I guess 6637ccd (Keep an fd for the cwd in the parser, 2019-06-10) was mainly for future use of openat().
Keeping cwd_fd open also means the OS will keep the inode valid (even when the associated path is removed/moved) but since we don't use it anywhere, I don't think there's a point in keeping it.
| } | ||
| if let Err(err) = open_dir(c".", BEST_O_SEARCH) { | ||
| perror_nix("Unable to open the current working directory", err); | ||
| } |
There was a problem hiding this comment.
this seems also unnecessary now. Will remove.
Of course in future we might want to behave like bash, i.e. mv $PWD /some/where does not change the output of ls etc. Not sure how much work that will be.
There was a problem hiding this comment.
this seems also unnecessary now
I had noticed but I left it in out of caution, in case this resulted in some extra checks (e.g. file permissions). But yes, I'm perfectly happy with it gone (and it gets rid of an untranslated string 😊).
Of course in future we might want to behave like bash, i.e.
mv $PWD /some/wheredoes not change the output oflsetc.
Bash does not keep $PWD opened either. ls works because it's equivalent to ls . not ls $PWD, i.e. relative path, resolved by the OS, rather than an absolute path, resolved by ls. And the OS can resolve because it keeps track of the pwd (/proc/pid/cwd) on its own. So this should still work fine with fish.
There was a problem hiding this comment.
Ockham's razor says this open_dir was just added for the reason given in 6637ccd (Keep an fd for the cwd in the parser, 2019-06-10), and not for any other extra checks
There was a problem hiding this comment.
So this should still work fine with fish.
right but cd . works in bash after $PWD has been renamed but not in fish. Maybe because fish tries to canonicalize paths.
There was a problem hiding this comment.
completion doesn't work in fish either, while it does in Bash (cat ./<tab>)
2 participants
sourcefish some_scriptwopen_cloexec/open_cloexec)TODOs:
Fixes issue #<issue-number>