Re: [PATCH v3 2/2] Use O_IGNORE_CTTY where appropriate

[Sergey Bugaev]

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