| Message ID | 20230604204258.2026816-3-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | O_IGNORE_CTTY everywhere | expand |
* Sergey Bugaev: > * getpt, openpty: Opening an unused pty, which can't be our ctty Please add this as a comment to the open operation. > * shm_open, sem_open: These don't work with ttys > * opendir: Directories are unlikely to be ttys I scrolled through the patch, and it seems to me that all these open calls could use O_NOCTTY. Do you still need a flag because the performance optimization is not about changing the controlling terminal, but about detecting the controlling terminal as such? Thanks, Florian
Hello, On Mon, Jun 5, 2023 at 12:28 PM Florian Weimer <fweimer@redhat.com> wrote: > > * shm_open, sem_open: These don't work with ttys > > * opendir: Directories are unlikely to be ttys > > I scrolled through the patch, and it seems to me that all these open > calls could use O_NOCTTY. Perhaps they could use O_NOCTTY indeed, though that would be a fix / change for correctness (on non-Hurd) and not a performance optimization. Would you prefer me to also add O_NOCTTY to all these calls? Or maybe I should even define O_IGNORE_CTTY to O_NOCTTY (instead of 0) on non-Hurd? Please note that O_NOCTTY is 0 on the Hurd, and opening a tty never makes it our ctty; you can only gain a ctty by explicitly requesting that (using TCIOSCTTY). An alternative way to think of this: O_NOCTTY is always in effect on the Hurd. > Do you still need a flag because the > performance optimization is not about changing the controlling terminal, > but about detecting the controlling terminal as such? Yes, exactly. The point of this patchset is skipping runtime ctty detection when we statically know for sure that the file being opened can not be our current ctty. This is not supposed to alter observable behavior, i.e. code that _can_ reasonably be reopening the current ctty should still work the same. But code that we know does not reopen our ctty should work faster, by skipping the check. Please also see the v1 cover letter [0]. [0]: https://sourceware.org/pipermail/libc-alpha/2023-April/147355.html It also helps to look with rpctrace at what this changes in practice. In case you don't readily have access to a Hurd system, I've uploaded the rpctrace of uname on Debian GNU/Hurd here: [1]. Unfortunately rpctrace output format is rather messy compared to strace; I've had a branch that attempted to improve that (along with many other fixes to rpctrace), but it's stalled/lost, so the current format will have to do for now. [1]: https://paste.gg/p/anonymous/68f3b1efa3384bf5807affde8fb83d83 You you grep / Ctrl-F for 'ctty', you can see there: 15<--28(pid1601)->term_getctty () = 0 4<--34(pid1601) task13(pid1601)->mach_port_deallocate (pn{ 10}) = 0 15<--28(pid1601)->term_open_ctty (0 0) = 0x40000016 (Invalid argument) 15<--28(pid1601)->term_getctty () = 0 4<--34(pid1601) task13(pid1601)->mach_port_deallocate (pn{ 10}) = 0 15<--28(pid1601)->term_open_ctty (0 0) = 0x40000016 (Invalid argument) 15<--28(pid1601)->term_getctty () = 0 4<--34(pid1601) task13(pid1601)->mach_port_deallocate (pn{ 10}) = 0 15<--28(pid1601)->term_open_ctty (0 0) = 0x40000016 (Invalid argument) This is the initial process startup. This is obviously broken -- it calls term_getctty/term_open_ctty on the same port (/dev/ttyp0 -- here, "15<--28(pid1601)") three times, getting an error each time! I've fixed the EINVALs in 346b6eab3c14ead0b716d53e2235464b822f48f2 "hurd: Run init_pids () before init_dtable ()". Also since the port for fds 0, 1, 2 is the same, it makes sense to only do the check once, and not once per fd -- that I have done in e55a55acb19400a26db4e7eec6d4649e364bc8d4 "hurd: Avoid extra ctty RPCs in init_dtable ()". But then later you find these: 12<--31(pid1601)->dir_lookup ("usr/lib/locale/C.utf8/LC_IDENTIFICATION" 4194305 0) = 0 1 "" 45<--22(pid1601) 45<--22(pid1601)->term_getctty () = 0xfffffed1 ((ipc/mig) bad request message ID) 12<--31(pid1601)->dir_lookup ("usr/lib/i386-gnu/gconv/gconv-modules.cache" 1 0) = 0 1 "" 45<--48(pid1601) 45<--48(pid1601)->term_getctty () = 0xfffffed1 ((ipc/mig) bad request message ID) (and many, many more). Fixing these is exactly what this patchset is about: we *know* that gconv-modules.cache is not our ctty; no need to check at runtime. Hope this makes sense! Sergey
On 2023-06-04 13:42, Sergey Bugaev via Libc-alpha wrote: > - int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC; > + int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_IGNORE_CTTY; Why is O_IGNORE_CTTY useful here? O_DIRECTORY means that the file cannot be a tty. (If GNU/Hurd is sending an extra RPC in this case, surely that's a performance bug that should be fixed in _hurd_port2fd or whatever, not in every 'open' caller.) > - open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC; > + open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_IGNORE_CTTY; Similarly, why is O_IGNORE_CTTY useful here? O_CREAT | O_EXCL means that the file cannot be a tty. (There are a couple of instances of this.) > I'm still interested in whether there could be a way to pass > O_IGNORE_CTTY when using fopen () too -- but that doesn't have to block > this patchset either. I suppose fopen could add a new 'T' flag, as a GNU extension, that would add O_IGNORE_CTTY to the open flags.
Hello, On Mon, Jun 5, 2023 at 11:58 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > > On 2023-06-04 13:42, Sergey Bugaev via Libc-alpha wrote: > > - int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC; > > + int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_IGNORE_CTTY; > > Why is O_IGNORE_CTTY useful here? O_DIRECTORY means that the file cannot > be a tty. (If GNU/Hurd is sending an extra RPC in this case, surely > that's a performance bug that should be fixed in _hurd_port2fd or > whatever, not in every 'open' caller.) > > > - open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC; > > + open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_IGNORE_CTTY; > > Similarly, why is O_IGNORE_CTTY useful here? O_CREAT | O_EXCL means that > the file cannot be a tty. (There are a couple of instances of this.) So what you're saying is the Hurd parts of glibc could/should try to infer O_IGNORE_CTTY in some cases based on other flags? That's a great point! -- especially because that would benefit other software that is not updated to pass O_IGNORE_CTTY explicitly. So: - O_DIRECTORY - O_CREAT | O_EXCL - O_TMPFILE, too should imply O_IGNORE_CTTY. I'm not very sure where this should be handled: _hurd_port2fd and _hurd_intern_fd are both public APIs, documented with "FLAGS are as for `open'; only O_IGNORE_CTTY and O_CLOEXEC are meaningful" so probably it's the code that calls _hurd_intern_fd that should do the conversion; but then there are several variants of open () alone (__libc_open, __open_nocancel, __openat, __openat_nocancel), and it would make sense to share the logic between them. Maybe I should make a new helper for this in fcntl-internal.h. But also, regardless of potentially inferring O_IGNORE_CTTY from other flags, I think it's a good practice to always pass O_IGNORE_CTTY where it makes sense to -- a lot like it's a good practice to always pass O_CLOEXEC. Well, in case of O_CLOEXEC it's a historical mistake that the default is not the other way around and a flag has to be passed explicitly to opt in, whereas O_IGNORE_CTTY is only appropriate in some cases, and the default should indeed be doing the checks for ctty. But when you know that you're not reopening your ctty I do think it makes sense to pass O_IGNORE_CTTY, even if it would be inferred otherwise. On the other hand I'm under no illusion that I'd be able to convince various third-party projects to use a Hurd-specific flag. The ones that I would like to see updated most are gcc/cpp (opening headers) and git (opening files in .git/ and the working tree) because they both open files in bulk: $ rpctrace gcc --version |& grep -c ctty 12 $ rpctrace gcc hello.c |& grep -c ctty 145 $ rpctrace git --version |& grep -c ctty 12 $ rpctrace git status |& grep -c ctty 158 Maybe with GCC there is a chance, considering it's a GNU project? > > I'm still interested in whether there could be a way to pass > > O_IGNORE_CTTY when using fopen () too -- but that doesn't have to block > > this patchset either. > > I suppose fopen could add a new 'T' flag, as a GNU extension, that would > add O_IGNORE_CTTY to the open flags. What would be the compatibility implications of this? -- what if POSIX later declares that 'T' must mean something else? I was thinking it could be possible to add some character without making it a public API/extension, and only using it inside glibc. Sergey
On 2023-06-06 02:21, Sergey Bugaev wrote: > _hurd_port2fd and > _hurd_intern_fd are both public APIs, documented with > > "FLAGS are as for `open'; only O_IGNORE_CTTY and O_CLOEXEC are meaningful" > You could change the documentation so that it now says that flags that imply O_IGNORE_CTTY are also meaningful. That should be fine. > I think it's a good practice to always pass O_IGNORE_CTTY where > it makes sense to Does your patch do that, for every 'open'-like call? > Maybe with GCC there is a chance, considering it's a GNU project? Sure. I expect even 'git' will do it if you write the patch, as they care about performance. Also tar, coreutils, grep, and other "open files a lot" apps should benefit. >> I suppose fopen could add a new 'T' flag, as a GNU extension, that would >> add O_IGNORE_CTTY to the open flags. > > What would be the compatibility implications of this? -- what if POSIX > later declares that 'T' must mean something else? I wouldn't worry overmuch about that. We could ask for forgiveness later. Thinking bigger - why does Hurd mess with this stuff at all? Wouldn't it be better if O_IGNORE_CTTY was the default, and a different flag (O_PAY_ATTENTION_TO_CTTY, say :-) enables the ctty dance? POSIX would allow that behavior, as it does not require the controlling terminal to be required if O_NOCTTY is not set.
Hello -- that is a much more positive response than I was expecting to get! Thank you! On Fri, Jun 9, 2023 at 9:37 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > You could change the documentation so that it now says that flags that > imply O_IGNORE_CTTY are also meaningful. That should be fine. Perhaps... but there's another reason I don't particularly like the idea of doing it on that level. _hurd_port2fd () and _hurd_intern_fd () is something that you call once you already have an io port. O_CREAT and O_DIRECTORY and the rest are the flags that impact how you look it up. _hurd_port2fd would have to second-guess "this io port is said to have been opened with O_CREAT | O_EXCL, so it can't be a ctty". It'd be better to have the caller (open) -- that "can see" both looking the port up and interning it -- implement this bit of logic. Not that this matters for anything, because it would still behave the same way no matter which level we implement it at; but it seems more appropriate to me to implement it at that level. Samuel, what do you think? > > I think it's a good practice to always pass O_IGNORE_CTTY where > > it makes sense to > > Does your patch do that, for every 'open'-like call? It does. I grepped glibc for open[at]64[_nocancel] and added O_IGNORE_CTTY everywhere where it made sense, other than in Linux-specific code (e.g. not inside sysdeps/unix/sysv/linux). I have also found a bunch of places where O_CLOEXEC was missing, and that lead to commit 533deafbdf189f5fbb280c28562dd43ace2f4b0f "Use O_CLOEXEC in more places (BZ #15722)". It might be that I have missed some more places where O_IGNORE_CTTY could be used, of course. > > Maybe with GCC there is a chance, considering it's a GNU project? > > Sure. I expect even 'git' will do it if you write the patch, as they > care about performance. Also tar, coreutils, grep, and other "open files > a lot" apps should benefit. Cool, so I should try writing the patches then and see what comes out of it. > >> I suppose fopen could add a new 'T' flag, as a GNU extension, that would > >> add O_IGNORE_CTTY to the open flags. > > > > What would be the compatibility implications of this? -- what if POSIX > > later declares that 'T' must mean something else? > > I wouldn't worry overmuch about that. We could ask for forgiveness later. I'm going to look into adding it into fopen then, thanks! > Thinking bigger - why does Hurd mess with this stuff at all? Wouldn't it > be better if O_IGNORE_CTTY was the default, and a different flag > (O_PAY_ATTENTION_TO_CTTY, say :-) enables the ctty dance? POSIX would > allow that behavior, as it does not require the controlling terminal to > be required if O_NOCTTY is not set. Sorry, I'm failing to parse/interpret that last sentence, could you please rephrase it? Do you perhaps mean that POSIX does not require a newly opened terminal to become your ctty even if you don't pass O_NOCTTY? Please note (if you haven't already) the difference between O_NOCTTY and O_IGNORE_CTTY: O_NOCTTY is "always in effect" on the Hurd, and defined to 0 (so I sure hope POSIX allows this). What O_IGNORE_CTTY does is -- well, technically it just turns off one check; but if you *don't* pass O_IGNORE_CTTY and the check succeeds and detects that the file you're opening (or rather: the port you're interning) refers to your ctty (that you already have set), then it does special things to make the new fd behave like an fd to your ctty is supposed to behave -- you e.g. get SIGINT when ^C is typed, and you get SIGTTIN / SIGTTOU / EBACKGROUND when trying to do I/O on the fd while in background. So if you do pass O_IGNORE_CTTY and the file is not your ctty, you just get a speedup. If you do pass O_IGNORE_CTTY and the file is your ctty, you get an fd that refers to your ctty... but doesn't behave like a ctty fd. Why you would want the latter, I have no idea (but also it of course wasn't me who came up with O_IGNORE_CTTY, so perhaps there is a use case). So it is important that (re)opening /dev/tty or /dev/stdout or just /dev/ttyp1 (by an explicit name) behaves as expected. Requiring an explicit flag for this would be wrong, and also there'd be even less of a chance that anyone would actually pass it in practice than there is now with O_IGNORE_CTTY. Sergey
On 6/9/23 14:13, Sergey Bugaev wrote: > Perhaps... but there's another reason I don't particularly like the > idea of doing it on that level. Yes, your points make sense. No big deal either way, > Do you perhaps mean that POSIX does not require a > newly opened terminal to become your ctty even if you don't pass > O_NOCTTY? Yes, that's right. The openat rationale mentions this topic. > So if you do pass O_IGNORE_CTTY and the file is not your ctty, you > just get a speedup. If you do pass O_IGNORE_CTTY and the file is your > ctty, you get an fd that refers to your ctty... but doesn't behave > like a ctty fd. Why you would want the latter, I have no idea (but > also it of course wasn't me who came up with O_IGNORE_CTTY, so perhaps > there is a use case). I don't see why anybody would care if the O_IGNORE_CTTY behavior became the default. And if nobody cares, let's just make it the default. That way, you won't have to change glibc, Gnulib, git, coreutils, etc. Do you have a scenario whereby making O_IGNORE_CTTY the default would break things? (It wouldn't break things as far as POSIX is concerned.)
Hello, On Sat, Jun 10, 2023 at 12:36 AM Paul Eggert <eggert@cs.ucla.edu> wrote: > > Do you perhaps mean that POSIX does not require a > > newly opened terminal to become your ctty even if you don't pass > > O_NOCTTY? > > Yes, that's right. The openat rationale mentions this topic. do you mean this?: "The O_NOCTTY flag was added to allow applications to avoid unintentionally acquiring a controlling terminal as a side-effect of opening a terminal file. This volume of POSIX.1-2017 does not specify how a controlling terminal is acquired, but it allows an implementation to provide this on open() if the O_NOCTTY flag is not set and other conditions specified in XBD General Terminal Interface are met." That paragraph is indeed about O_NOCTTY and acquiring a ctty if you don't yet have one. It says nothing about whether the O_IGNORE_CTTY behavior is allowed. To reiterate: O_IGNORE_CTTY is not about acquiring a ctty if you don't yet have one (that never happens implicitly on the Hurd), but about (re)opening your current ctty. > I don't see why anybody would care if the O_IGNORE_CTTY behavior became > the default. And if nobody cares, let's just make it the default. That > way, you won't have to change glibc, Gnulib, git, coreutils, etc. > > Do you have a scenario whereby making O_IGNORE_CTTY the default would > break things? (It wouldn't break things as far as POSIX is concerned.) But it would break things as far as POSIX is concerned: see the description of how I/O on a ctty should behave (namely generating SIGTTIN / SIGTTOU for a background process) in "11.1.4 Terminal Access Control". I don't know whether any programs actually care about this ctty feature. Presumably users care? -- as I understand it, this is intended to be used with job control in the shell, so you can launch some long-running batch job in the background, and have it stopped with SIGTTIN when it tries to read from your terminal (instead of it fighting for input with whatever foreground job at the time); you can then resume it and give it the desired input. Although this all (job control as a whole) has largely faded away with the proliferation of tabs in terminal emulators. But it still works, and is supposed to work, on today's Unixes, and is required to work by POSIX. Also I don't think it'd be possible to flip the default like that even if we wanted to. O_IGNORE_CTTY is not some new feature, it's been in glibc as far back as Git history goes ("initial import" in 1995 -- it's one of those many things about the Hurd that are older than me :). This has been the behavior and a public API for 25+ years, we cannot just change it to the opposite now. Sergey
On 6/10/23 12:13, Sergey Bugaev wrote: > O_IGNORE_CTTY is not about > acquiring a ctty if you don't yet have one (that never happens > implicitly on the Hurd), but about (re)opening your current ctty. OK, I'm starting to see the distinction now. > I don't know whether any programs actually care about this ctty > feature. Presumably users care? -- as I understand it, this is > intended to be used with job control in the shell If so, it's a well-kept secret, as Bash doesn't use O_IGNORE_CTTY. The only program I know of that uses O_IGNORE_CTTY is Emacs, and it's for what appears to be relatively minor optimization when it is opening a file that is a tty. On non-Hurd platforms Emacs instead uses setsid to remove the controlling tty entirely (since the notion of controlling terminal doesn't mix well with how Emacs operates).
Hello, On Sun, Jun 11, 2023 at 7:54 AM Paul Eggert <eggert@cs.ucla.edu> wrote: > OK, I'm starting to see the distinction now. So you did misunderstand it! That means not only is the explanation in the glibc manual (reproduced below) unclear, but my previous attempts to describe it and its differences to O_NOCTTY were unclear as well. "Do not recognize the named file as the controlling terminal, even if it refers to the process’s existing controlling terminal device. Operations on the new file descriptor will never induce job control signals." This is an opportunity to improve the docs! -- please tell me how we could improve it so it would have been clear to you from the start. And in any case I should add a blurb about it making open faster and a recommendation to use it whenever possible. > > I don't know whether any programs actually care about this ctty > > feature. Presumably users care? -- as I understand it, this is > > intended to be used with job control in the shell > > If so, it's a well-kept secret, as Bash doesn't use O_IGNORE_CTTY. There seems to be a miscommunication here as well: I'm replying to your question of whether anybody cares about O_IGNORE_CTTY behavior *not* being the default, i.e. the default behavior being the opposite (honoring ctty). Programs don't actively use O_IGNORE_CTTY to get that behavior exactly because they get that behavior by default. So you can't get an estimate for whether any software cares by grepping it for O_IGNORE_CTTY. The "this" in my "this is intended to be used with job control in the shell" refers to the feature of cttys that a background process trying to do I/O on the terminal results in a signal (and then an error if the signal is ignored); and not to O_IGNORE_CTTY (which turns this feature *off*). > The only program I know of that uses O_IGNORE_CTTY is Emacs, and it's > for what appears to be relatively minor optimization when it is opening > a file that is a tty. On non-Hurd platforms Emacs instead uses setsid to > remove the controlling tty entirely (since the notion of controlling > terminal doesn't mix well with how Emacs operates). Speaking of Emacs, and seeing that Emacs already defines O_IGNORE_CTTY to 0 on non-Hurd, and that you're one of the Emacs maintainers (is that right?): could perhaps Emacs benefit from using O_IGNORE_CTTY more broadly too? I imagine loading all the .el files in ~/.emacs.d involves way too many pointless ctty checks, for example. Sergey
On 6/13/23 03:54, Sergey Bugaev wrote: > "Do not recognize the named file as the controlling terminal, even if > it refers to the process’s existing controlling terminal device. > Operations on the new file descriptor will never induce job control > signals." > > This is an opportunity to improve the docs!7 The first-quoted sentence is confusing and arguably wrong. If the named file is the process's existing controlling terminal, it will continue to be recognized as the controlling terminal. What's different is that I/O to the new fd won't induce SIGTTIN etc. So the second sentence is correct and the first is wrong. Perhaps the first sentence could be replaced by: "Cause operations on the new file descriptor to act as if the named file is not the process's controlling terminal, even if it is." > you > can't get an estimate for whether any software cares by grepping it > for O_IGNORE_CTTY. Fair enough. Still, I still doubt many would care if O_IGNORE_CTTY became the default, particularly since O_NOCTTY is 0. What's the scenario whereby a Bash user would care, for example? > Speaking of Emacs, and seeing that Emacs already defines O_IGNORE_CTTY > to 0 on non-Hurd, and that you're one of the Emacs maintainers (is > that right?): could perhaps Emacs benefit from using O_IGNORE_CTTY > more broadly too? I imagine loading all the .el files in ~/.emacs.d > involves way too many pointless ctty checks, for example. It might, I suppose. Got a patch?
Hello, On Wed, Jun 14, 2023 at 8:40 AM Paul Eggert <eggert@cs.ucla.edu> wrote: > The first-quoted sentence is confusing and arguably wrong. If the named > file is the process's existing controlling terminal, it will continue to > be recognized as the controlling terminal. Thanks, I can see how that can sound confusing: the *file* will continue being recognized as the ctty, but the newly opened fd won't be recognized as one that refers to a ctty. > What's different is that I/O to the new fd won't induce SIGTTIN etc. So > the second sentence is correct and the first is wrong. Perhaps the first > sentence could be replaced by: > > "Cause operations on the new file descriptor to act as if the named file > is not the process's controlling terminal, even if it is." So how about this? "Cause operations on the new file descriptor to act as if the named file is not the process's controlling terminal, even if it is. @xref{Job Control}. When @code{O_IGNORE_CTTY} is not set, @code{open} has to perform a runtime check for the named file being the process's controlling terminal; setting @code{O_IGNORE_CTTY} allows @code{open} to skip this check. In case the named file is statically known not to be the process's controlling terminal, for example when opening a configuration or a cache file using a well-known path, setting @code{O_IGNORE_CTTY} will lead to improved @code{open} performance and no behavior change. For this reason, it is good practice to always set @code{O_IGNORE_CTTY} when opening files, unless there is a possibility that the file being opened could be the process's controlling terminal." > Still, I still doubt many would care if O_IGNORE_CTTY > became the default, particularly since O_NOCTTY is 0. What's the > scenario whereby a Bash user would care, for example? They would run a program/job in the background, and wouldn't want input or output from the background program to mess up what they're doing. With !O_IGNORE_CTTY, when the background program attempts to do I/O on the terminal (only input by default, both with 'stty tostop'), its gets stopped, and Bash reports: [1]+ pid-here Stopped (tty input) The user would then resume it with 'fg %' (or just '%') when they're ready, and it would do its I/O then. This is described in "Unix Power Tools" [0], the Bash manual [1], and Zsh guide [2], so it's not even that obscure of a feature. [0]: https://docstore.mik.ua/orelly/unix3/upt/ch23_09.htm [1]: https://www.gnu.org/software/bash/manual/html_node/Job-Control-Basics.html [2]: https://zsh.sourceforge.io/Guide/zshguide03.html#l39 Although O_IGNORE_CTTY would only matter if the program reopens the tty explicitly, perhaps as /dev/tty or /dev/stdout, not for the file descriptors inherited across exec. sudo does this (reopening the terminal), for example, so if you have a 'sudo xxxx' line in a script that you run as a background job, it'd steal your input if O_IGNORE_CTTY behavior was the default. > > Speaking of Emacs, and seeing that Emacs already defines O_IGNORE_CTTY > > to 0 on non-Hurd, and that you're one of the Emacs maintainers (is > > that right?): could perhaps Emacs benefit from using O_IGNORE_CTTY > > more broadly too? I imagine loading all the .el files in ~/.emacs.d > > involves way too many pointless ctty checks, for example. > > It might, I suppose. Got a patch? Hmm, I see there's a emacs_open[at] wrapper that automatically adds O_CLOEXEC (and also O_BINARY). So now I've got the same question for you: does Emacs ever care about the default, !O_IGNORE_CTTY behavior? Would anything break if I just make emacs_openat always add in O_IGNORE_CTTY? OT1H the answer surely must be no, since Emacs is an interactive editor, it doesn't just read and write its stdin/stdout/stderr as streams like classic Unix utilities do. OTOH I see that it does deal with cttys and SIGTTOU in several places... Sergey
On 2023-06-16 09:26, Sergey Bugaev wrote: > Hello, > So how about this? > > "Cause operations on the new file descriptor to act as if the named > file is not the process's controlling terminal, even if it is. > @xref{Job Control}. > > When @code{O_IGNORE_CTTY} is not set, @code{open} has to perform a > runtime check for the named file being the process's controlling > terminal; setting @code{O_IGNORE_CTTY} allows @code{open} to skip this > check. In case the named file is statically known not to be the "In case the named file is statically known not to" -> "If the named file cannot" > @code{O_IGNORE_CTTY} will lead to improved @code{open} performance and > no behavior change. For this reason, it is good practice to always > set @code{O_IGNORE_CTTY} when opening files, unless there is a > possibility that the file being opened could be the process's > controlling terminal." Replace this with just "@code{O_IGNORE_CTTY} improves performance on the Hurd." as the rest is redundant. > Although O_IGNORE_CTTY would only matter if the program reopens the tty > explicitly, perhaps as /dev/tty or /dev/stdout, not for the file > descriptors inherited across exec. sudo does this (reopening the > terminal), for example, so if you have a 'sudo xxxx' line in a script > that you run as a background job, it'd steal your input if O_IGNORE_CTTY > behavior was the default. Fine, so add an O_KEEP_CTTY flag for programs like sudo that want to play tricks with /dev/tty, and add a feature-test macro like _DEFAULT_IGNORE_CTTY to let applications choose whether O_IGNORE_CTTY or O_KEEP_CTTY is the default. If done right this would be an upward compatible API and ABI change, and would let people fix their apps with a simple '#define _DEFAULT_IGNORE_CTTY 1' before their other #include directives, instead of wandering through all their source code looking for places to add O_IGNORE_CTTY here and there. > Hmm, I see there's a emacs_open[at] wrapper that automatically adds > O_CLOEXEC (and also O_BINARY). So now I've got the same question for > you: does Emacs ever care about the default, !O_IGNORE_CTTY behavior? > Would anything break if I just make emacs_openat always add in > O_IGNORE_CTTY? Haven't a clue. Why don't you try it and run it for a while? But better yet, let's change the API as described above, and then try that with Emacs.
Hello, Sergey Bugaev, le sam. 10 juin 2023 00:13:22 +0300, a ecrit: > On Fri, Jun 9, 2023 at 9:37 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > > You could change the documentation so that it now says that flags that > > imply O_IGNORE_CTTY are also meaningful. That should be fine. > > Perhaps... but there's another reason I don't particularly like the > idea of doing it on that level. > > _hurd_port2fd () and _hurd_intern_fd () is something that you call > once you already have an io port. O_CREAT and O_DIRECTORY and the rest > are the flags that impact how you look it up. _hurd_port2fd would have > to second-guess "this io port is said to have been opened with O_CREAT > | O_EXCL, so it can't be a ctty". It'd be better to have the caller > (open) -- that "can see" both looking the port up and interning it -- > implement this bit of logic. Not that this matters for anything, > because it would still behave the same way no matter which level we > implement it at; but it seems more appropriate to me to implement it > at that level. > > Samuel, what do you think? It looks better to me to add a shared helper that adds O_IGNORE_CTTY whenever the flags contain something that implies it. Callers of _hurd_intern_fd / _hurd_port2fd can then easily use it (or even just always pass O_IGNORE_CTTY, when creating a socket for instance). Samuel
Hello, On Mon, Jun 19, 2023 at 12:55 AM Samuel Thibault <samuel.thibault@gnu.org> wrote: > > Samuel, what do you think? > > It looks better to me to add a shared helper that adds O_IGNORE_CTTY > whenever the flags contain something that implies it. Callers of > _hurd_intern_fd / _hurd_port2fd can then easily use it That's exactly what I was thinking, thank you. > (or even just > always pass O_IGNORE_CTTY, when creating a socket for instance). It does that already, yes. Sergey
diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c index 46c444d2..129f4662 100644 --- a/catgets/open_catalog.c +++ b/catgets/open_catalog.c @@ -49,7 +49,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, char *buf = NULL; if (strchr (cat_name, '/') != NULL || nlspath == NULL) - fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC); + fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); else { const char *run_nlspath = nlspath; @@ -177,7 +177,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, if (bufact != 0) { - fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC); + fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); if (fd >= 0) break; } diff --git a/elf/dl-load.c b/elf/dl-load.c index 9a87fda9..f58fa95e 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1620,7 +1620,7 @@ open_verify (const char *name, int fd, if (fd == -1) /* Open the file. We always open files read-only. */ - fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC); + fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); if (fd != -1) { diff --git a/elf/dl-misc.c b/elf/dl-misc.c index 5b84adc2..85931c7c 100644 --- a/elf/dl-misc.c +++ b/elf/dl-misc.c @@ -36,7 +36,7 @@ _dl_sysdep_read_whole_file (const char *file, size_t *sizep, int prot) { void *result = MAP_FAILED; struct __stat64_t64 st; - int fd = __open64_nocancel (file, O_RDONLY | O_CLOEXEC); + int fd = __open64_nocancel (file, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); if (fd >= 0) { if (__fstat64_time64 (fd, &st) >= 0) diff --git a/elf/dl-profile.c b/elf/dl-profile.c index 8be0065f..040734d1 100644 --- a/elf/dl-profile.c +++ b/elf/dl-profile.c @@ -325,7 +325,7 @@ _dl_start_profile (void) __stpcpy (__stpcpy (cp, GLRO(dl_profile)), ".profile"); fd = __open64_nocancel (filename, O_RDWR | O_CREAT | O_NOFOLLOW - | O_CLOEXEC, DEFFILEMODE); + | O_CLOEXEC | O_IGNORE_CTTY, DEFFILEMODE); if (fd == -1) { char buf[400]; diff --git a/gmon/gmon.c b/gmon/gmon.c index 6439ed1c..ed13b3ce 100644 --- a/gmon/gmon.c +++ b/gmon/gmon.c @@ -385,13 +385,13 @@ write_gmon (void) char buf[len + 20]; __snprintf (buf, sizeof (buf), "%s.%u", env, __getpid ()); fd = __open_nocancel (buf, O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW - | O_CLOEXEC, 0666); + | O_CLOEXEC | O_IGNORE_CTTY, 0666); } if (fd == -1) { fd = __open_nocancel ("gmon.out", O_CREAT | O_TRUNC | O_WRONLY - | O_NOFOLLOW | O_CLOEXEC, 0666); + | O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY, 0666); if (fd < 0) { char buf[300]; diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c index 87136e24..c8d972c8 100644 --- a/iconv/gconv_cache.c +++ b/iconv/gconv_cache.c @@ -58,7 +58,8 @@ __gconv_load_cache (void) return -1; /* See whether the cache file exists. */ - fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY | O_CLOEXEC, 0); + fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY + | O_CLOEXEC | O_IGNORE_CTTY, 0); if (__builtin_expect (fd, 0) == -1) /* Not available. */ return -1; diff --git a/locale/loadarchive.c b/locale/loadarchive.c index 5b857d5d..f88ff8b8 100644 --- a/locale/loadarchive.c +++ b/locale/loadarchive.c @@ -202,7 +202,8 @@ _nl_load_locale_from_archive (int category, const char **namep) archmapped = &headmap; /* The archive has never been opened. */ - fd = __open_nocancel (archfname, O_RDONLY|O_LARGEFILE|O_CLOEXEC); + fd = __open_nocancel (archfname, O_RDONLY | O_LARGEFILE + | O_CLOEXEC | O_IGNORE_CTTY); if (fd < 0) /* Cannot open the archive, for whatever reason. */ return NULL; @@ -397,8 +398,8 @@ _nl_load_locale_from_archive (int category, const char **namep) if (fd == -1) { struct __stat64_t64 st; - fd = __open_nocancel (archfname, - O_RDONLY|O_LARGEFILE|O_CLOEXEC); + fd = __open_nocancel (archfname, O_RDONLY | O_LARGEFILE + | O_CLOEXEC | O_IGNORE_CTTY); if (fd == -1) /* Cannot open the archive, for whatever reason. */ return NULL; diff --git a/locale/loadlocale.c b/locale/loadlocale.c index 671e71cf..582144ed 100644 --- a/locale/loadlocale.c +++ b/locale/loadlocale.c @@ -240,7 +240,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category) file->decided = 1; file->data = NULL; - fd = __open_nocancel (file->filename, O_RDONLY | O_CLOEXEC); + fd = __open_nocancel (file->filename, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); if (__builtin_expect (fd, 0) < 0) /* Cannot open the file. */ return; @@ -267,7 +267,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category) "/SYS_", 5), _nl_category_names_get (category), _nl_category_name_sizes[category] + 1); - fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC); + fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY); if (__builtin_expect (fd, 0) < 0) return; diff --git a/login/openpty.c b/login/openpty.c index 1e44852a..a89555b2 100644 --- a/login/openpty.c +++ b/login/openpty.c @@ -117,7 +117,7 @@ __openpty (int *pptmx, int *pterminal, char *name, if (pts_name (ptmx, &buf, sizeof (_buf))) goto on_error; - terminal = __open64 (buf, O_RDWR | O_NOCTTY); + terminal = __open64 (buf, O_RDWR | O_NOCTTY | O_IGNORE_CTTY); if (terminal == -1) goto on_error; } diff --git a/login/utmp_file.c b/login/utmp_file.c index 7055041d..bdf88b51 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -142,7 +142,7 @@ __libc_setutent (void) file_writable = false; file_fd = __open_nocancel - (file_name, O_RDONLY | O_LARGEFILE | O_CLOEXEC); + (file_name, O_RDONLY | O_LARGEFILE | O_CLOEXEC | O_IGNORE_CTTY); if (file_fd == -1) return 0; } @@ -354,7 +354,7 @@ __libc_pututline (const struct utmp *data) const char *file_name = TRANSFORM_UTMP_FILE_NAME (__libc_utmp_file_name); int new_fd = __open_nocancel - (file_name, O_RDWR | O_LARGEFILE | O_CLOEXEC); + (file_name, O_RDWR | O_LARGEFILE | O_CLOEXEC | O_IGNORE_CTTY); if (new_fd == -1) return NULL; @@ -463,7 +463,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp) int fd; /* Open WTMP file. */ - fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE | O_CLOEXEC); + fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE + | O_CLOEXEC | O_IGNORE_CTTY); if (fd < 0) return -1; diff --git a/misc/daemon.c b/misc/daemon.c index 14577e40..2f653ec7 100644 --- a/misc/daemon.c +++ b/misc/daemon.c @@ -67,7 +67,7 @@ daemon (int nochdir, int noclose) { struct __stat64_t64 st; - fd = __open_nocancel (_PATH_DEVNULL, O_RDWR, 0); + fd = __open_nocancel (_PATH_DEVNULL, O_RDWR | O_IGNORE_CTTY, 0); if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) { if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0 diff --git a/nss/nss_db/db-open.c b/nss/nss_db/db-open.c index a5279dc0..2a996a6e 100644 --- a/nss/nss_db/db-open.c +++ b/nss/nss_db/db-open.c @@ -36,7 +36,8 @@ internal_setent (const char *file, struct nss_db_map *mapping) { enum nss_status status = NSS_STATUS_UNAVAIL; - int fd = __open_nocancel (file, O_RDONLY | O_LARGEFILE | O_CLOEXEC); + int fd = __open_nocancel (file, O_RDONLY | O_LARGEFILE + | O_CLOEXEC | O_IGNORE_CTTY); if (fd != -1) { struct nss_db_header header; diff --git a/rt/shm_open.c b/rt/shm_open.c index fc1dc96b..7fd62cf3 100644 --- a/rt/shm_open.c +++ b/rt/shm_open.c @@ -37,7 +37,7 @@ __shm_open (const char *name, int oflag, mode_t mode) return -1; } - oflag |= O_NOFOLLOW | O_CLOEXEC; + oflag |= O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY; #if defined (SHM_ANON) && defined (O_TMPFILE) if (name == SHM_ANON) oflag |= O_TMPFILE; diff --git a/shadow/lckpwdf.c b/shadow/lckpwdf.c index 3b36b2eb..4a623c41 100644 --- a/shadow/lckpwdf.c +++ b/shadow/lckpwdf.c @@ -96,7 +96,7 @@ __lckpwdf (void) /* Prevent problems caused by multiple threads. */ __libc_lock_lock (lock); - int oflags = O_WRONLY | O_CREAT | O_CLOEXEC; + int oflags = O_WRONLY | O_CREAT | O_CLOEXEC | O_IGNORE_CTTY; lock_fd = __open (PWD_LOCKFILE, oflags, 0600); if (lock_fd == -1) /* Cannot create lock file. */ diff --git a/sysdeps/mach/hurd/opendir.c b/sysdeps/mach/hurd/opendir.c index 39c805bc..07260d22 100644 --- a/sysdeps/mach/hurd/opendir.c +++ b/sysdeps/mach/hurd/opendir.c @@ -73,7 +73,7 @@ __opendirat (int dfd, const char *name) but `open' might like it fine. */ return __hurd_fail (ENOENT), NULL; - int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC; + int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_IGNORE_CTTY; int fd; #if IS_IN (rtld) assert (dfd == AT_FDCWD); diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c index e5db929d..5a248ebb 100644 --- a/sysdeps/pthread/sem_open.c +++ b/sysdeps/pthread/sem_open.c @@ -65,7 +65,7 @@ __sem_open (const char *name, int oflag, ...) /* If the semaphore object has to exist simply open it. */ if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0) { - open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC; + open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY; open_flags |= (oflag & ~(O_CREAT|O_ACCMODE)); try_again: fd = __open (dirname.name, open_flags); @@ -135,7 +135,7 @@ __sem_open (const char *name, int oflag, ...) } /* Open the file. Make sure we do not overwrite anything. */ - open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC; + open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_IGNORE_CTTY; fd = __open (tmpfname, open_flags, mode); if (fd == -1) { diff --git a/sysdeps/unix/bsd/getpt.c b/sysdeps/unix/bsd/getpt.c index 8369f958..48f3d07a 100644 --- a/sysdeps/unix/bsd/getpt.c +++ b/sysdeps/unix/bsd/getpt.c @@ -76,7 +76,7 @@ __bsd_openpt (int oflag) int __getpt (void) { - return __bsd_openpt (O_RDWR); + return __bsd_openpt (O_RDWR | O_IGNORE_CTTY); } libc_hidden_def (__getpt) weak_alias (__getpt, getpt) @@ -84,6 +84,6 @@ weak_alias (__getpt, getpt) int __posix_openpt (int oflag) { - return __bsd_openpt (oflag); + return __bsd_openpt (oflag | O_IGNORE_CTTY); } weak_alias (__posix_openpt, posix_openpt)
* getpt, openpty: Opening an unused pty, which can't be our ctty * shm_open, sem_open: These don't work with ttys * opendir: Directories are unlikely to be ttys Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- catgets/open_catalog.c | 4 ++-- elf/dl-load.c | 2 +- elf/dl-misc.c | 2 +- elf/dl-profile.c | 2 +- gmon/gmon.c | 4 ++-- iconv/gconv_cache.c | 3 ++- locale/loadarchive.c | 7 ++++--- locale/loadlocale.c | 4 ++-- login/openpty.c | 2 +- login/utmp_file.c | 7 ++++--- misc/daemon.c | 2 +- nss/nss_db/db-open.c | 3 ++- rt/shm_open.c | 2 +- shadow/lckpwdf.c | 2 +- sysdeps/mach/hurd/opendir.c | 2 +- sysdeps/pthread/sem_open.c | 4 ++-- sysdeps/unix/bsd/getpt.c | 4 ++-- 17 files changed, 30 insertions(+), 26 deletions(-)