[PATCH v3 0/2] O_IGNORE_CTTY everywhere

[PATCH v3 0/2] O_IGNORE_CTTY everywhere

[Sergey Bugaev]

Hello,

this is the v3 of the "O_IGNORE_CTTY everywhere" patchset. Please see
the v1 cover letter [0] for an explanation of what O_IGNORE_CTTY is and
does.

[0]: https://sourceware.org/pipermail/libc-alpha/2023-April/147355.html

Changes since v2:

1. Dropped the already commited patches.

2. Dropped the unrelated changes to daemon and csu/check_fds.c, since
there doesn't seem to be a clear consensus on what we want there. I
don't really care that much about those changes, and I don't think they
should stall/block the actual O_IGNORE_CTTY patches. Plus, the Hurd has
its own version of check_fds.c anyway, which already does the right
thing wrt cttys.

With this, the patchset is quite small and manageable again.

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.

Sergey

Sergey Bugaev (2):
  include/fcntl.h: Define O_IGNORE_CTTY
  Use O_IGNORE_CTTY where appropriate

 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 ++-
 include/fcntl.h             | 15 +++++++++++++++
 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 ++--
 18 files changed, 45 insertions(+), 26 deletions(-)

-- 
2.40.1

[PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Sergey Bugaev]

This internal definition makes it possible to use O_IGNORE_CTTY in
the glibc codebase unconditionally, no matter whether the current port
provides it or not (i.e. both on Hurd and on Linux). Along with the
definition, this adds a small guide on when O_IGNORE_CTTY is to be used.

The following commit will actually make use of O_IGNORE_CTTY
throughout the glibc codebase.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 include/fcntl.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/fcntl.h b/include/fcntl.h
index be435047..d788db2e 100644
--- a/include/fcntl.h
+++ b/include/fcntl.h
@@ -33,6 +33,21 @@ extern int __openat_2 (int __fd, const char *__path, int __oflag);
 extern int __openat64_2 (int __fd, const char *__path, int __oflag);
 
 
+/* Makes open () & friends faster on the Hurd, but can only be used (without
+   altering user-visible behavior) when we're sure that the file we're opening
+   is not (at the moment) our controlling terminal.  Use this when:
+   - opening well-known files internally (utmp, nss db);
+   - opening files with user-specified names that can not reasonably be ttys
+     (sem_open, shm_open);
+   - opening new (previously unused) ttys (openpty).
+   Don't use this when:
+   - doing a general-purpose open () with a user-controlled path that could
+     well be "/dev/tty" (fopen).  */
+#ifndef O_IGNORE_CTTY
+#  define O_IGNORE_CTTY	0
+#endif
+
+
 #if IS_IN (rtld)
 #  include <dl-fcntl.h>
 #endif
-- 
2.40.1

[PATCH v3 2/2] Use O_IGNORE_CTTY where appropriate

[Sergey Bugaev]

* 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(-)

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)
-- 
2.40.1

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

[Florian Weimer]

* 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

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

[Sergey Bugaev]

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

Re: [PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Adhemerval Zanella Netto]

On 04/06/23 17:42, Sergey Bugaev via Libc-alpha wrote:
> This internal definition makes it possible to use O_IGNORE_CTTY in
> the glibc codebase unconditionally, no matter whether the current port
> provides it or not (i.e. both on Hurd and on Linux). Along with the
> definition, this adds a small guide on when O_IGNORE_CTTY is to be used.
> 
> The following commit will actually make use of O_IGNORE_CTTY
> throughout the glibc codebase.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  include/fcntl.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/include/fcntl.h b/include/fcntl.h
> index be435047..d788db2e 100644
> --- a/include/fcntl.h
> +++ b/include/fcntl.h
> @@ -33,6 +33,21 @@ extern int __openat_2 (int __fd, const char *__path, int __oflag);
>  extern int __openat64_2 (int __fd, const char *__path, int __oflag);
>  
>  
> +/* Makes open () & friends faster on the Hurd, but can only be used (without
> +   altering user-visible behavior) when we're sure that the file we're opening
> +   is not (at the moment) our controlling terminal.  Use this when:
> +   - opening well-known files internally (utmp, nss db);
> +   - opening files with user-specified names that can not reasonably be ttys
> +     (sem_open, shm_open);
> +   - opening new (previously unused) ttys (openpty).
> +   Don't use this when:
> +   - doing a general-purpose open () with a user-controlled path that could
> +     well be "/dev/tty" (fopen).  */
> +#ifndef O_IGNORE_CTTY
> +#  define O_IGNORE_CTTY	0
> +#endif
> +

I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which
defines O_IGNORE_CTTY  unconditionally and include the default one (either
directly or though include_next.h).  We currently are trying to avoid the 
"#ifdef ...", so a code that does not define, where is should, would fail 
at compile time.

> +
>  #if IS_IN (rtld)
>  #  include <dl-fcntl.h>
>  #endif

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

[Paul Eggert]

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.

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

Re: [PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Sergey Bugaev]

Hello,

On Mon, Jun 5, 2023 at 9:25 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> We currently are trying to avoid the
> "#ifdef ...", so a code that does not define, where is should, would fail
> at compile time.

Yes, this makes perfect sense, and it was something I was also
slightly concerned about (what if the Hurd's real definition stops
being brought in by include/fcntl.h for some reason? -- then we'd just
silently get a 0, and nobody would notice). On the other hand I wanted
to not cause any additional troubles for other potential ports
(FreeBSD), but maybe it's fine to require them to just add their own
little header.

Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY
and not to 0?

> I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which
> defines O_IGNORE_CTTY  unconditionally and include the default one (either
> directly or though include_next.h).

Could you please clarify how this whole system of file overrides
works? (I mean: a more specific sysdep file, for some unclear
definition of "specific", automatically overriding a less specific
file of the same basename.)

I think I've seen vpath get used somewhere, so I would guess that the
sysdep (and other) directories are added to vpath order by their
priority, and whichever one Make finds first, it passes to the
compiler. Header files, I would guess again, are simply handled by
passing all the paths (once again properly sorted) with -I, and it's
the compiler that looks for the first directory that contains file of
the given name -- this makes it possible to #include_next, and this
must also be how include/ contains headers that are used during libc
compilation but not installed (include/ must not be on the vpath
then?).

But this brings me to the more specific question: the headers to be
installed are also found using vpath during 'make install', right? How
would this work, will Make somehow know to not install this
sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep
installing io/fcntl.h?

Sergey

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

[Paul Eggert]

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.

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

[Sergey Bugaev]

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

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

[Paul Eggert]

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.)

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

[Sergey Bugaev]

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

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

[Paul Eggert]

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).

Re: [PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Adhemerval Zanella Netto]

On 09/06/23 06:29, Sergey Bugaev wrote:
> Hello,
> 
> On Mon, Jun 5, 2023 at 9:25 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>> We currently are trying to avoid the
>> "#ifdef ...", so a code that does not define, where is should, would fail
>> at compile time.
> 
> Yes, this makes perfect sense, and it was something I was also
> slightly concerned about (what if the Hurd's real definition stops
> being brought in by include/fcntl.h for some reason? -- then we'd just
> silently get a 0, and nobody would notice). On the other hand I wanted
> to not cause any additional troubles for other potential ports
> (FreeBSD), but maybe it's fine to require them to just add their own
> little header.

Yeah that's the idea, but by adding a generic one it would be required
iff the kernel/system needs to something differente.

> 
> Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY
> and not to 0?

Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic,
so I think we should avoid change the internal open flags in this
specific patch.

> 
>> I think it would be better to add a sysdeps/unix/sysv/linux/fcntl.h which
>> defines O_IGNORE_CTTY  unconditionally and include the default one (either
>> directly or though include_next.h).
> 
> Could you please clarify how this whole system of file overrides
> works? (I mean: a more specific sysdep file, for some unclear
> definition of "specific", automatically overriding a less specific
> file of the same basename.)

We have the internal header file, to say 'include/fcntl.h', which is
used when building glibc itself including the tests.  The internal-only
header also includes the installed one, 'io/fcntl.h', which defines
the ABI glibc provides.

So to override a internal-only definition we have some options:

  1. Override the file which is has precedence in the sysdeps selection
     (which defines the -I at built time).  So if you add the file
     "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included 
     instead of 'include/fcntl.h'.
     To avoid the need to replicate the same prototypes and definitions
     on the generic 'include/fcntl.h' in the system specific one we
     can use the include_next extension (check on how the sysvipc code
     done, like include/sys/sem.h).

  2. Add per-system file that is included in the generic 'include/fcntl.h',
     for instance fcntl-system.h or something like that.  On then add
     a generic definition on sysdep/generic/ with the expected value
     and override it on any sysdep folder that requires it.

I tend to see the second options as a slight simpler one.

> 
> I think I've seen vpath get used somewhere, so I would guess that the
> sysdep (and other) directories are added to vpath order by their
> priority, and whichever one Make finds first, it passes to the
> compiler. Header files, I would guess again, are simply handled by
> passing all the paths (once again properly sorted) with -I, and it's
> the compiler that looks for the first directory that contains file of
> the given name -- this makes it possible to #include_next, and this
> must also be how include/ contains headers that are used during libc
> compilation but not installed (include/ must not be on the vpath
> then?).
> 
> But this brings me to the more specific question: the headers to be
> installed are also found using vpath during 'make install', right? How
> would this work, will Make somehow know to not install this
> sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep
> installing io/fcntl.h?

Afaiu there is no need to install any new header for this, this is an
internal only definition to use on open call within glibc.

Re: [PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Sergey Bugaev]

Hello,

On Mon, Jun 12, 2023 at 9:56 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> > Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY
> > and not to 0?
>
> Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic,
> so I think we should avoid change the internal open flags in this
> specific patch.

OK.

> We have the internal header file, to say 'include/fcntl.h', which is
> used when building glibc itself including the tests.  The internal-only
> header also includes the installed one, 'io/fcntl.h', which defines
> the ABI glibc provides.

Yes, that I understand.

> So to override a internal-only definition we have some options:
>
>   1. Override the file which is has precedence in the sysdeps selection
>      (which defines the -I at built time).  So if you add the file
>      "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included
>      instead of 'include/fcntl.h'.

Oh cool, I didn't realize it was possible to nest include/ under
sysdeps/, thank you! I thought there was only the top-level include/
directory, but now I see that there are more, including even a few
Mach/Hurd specific ones.

>   2. Add per-system file that is included in the generic 'include/fcntl.h',
>      for instance fcntl-system.h or something like that.  On then add
>      a generic definition on sysdep/generic/ with the expected value
>      and override it on any sysdep folder that requires it.

Is there already an example of such a header file? I can't find any
files in any include/ directory matching either *sysdep* or *system*.

> I tend to see the second options as a slight simpler one.

The first option sounds simpler to me.

Should it be sysdeps/unix/sysv/linux/include/fcntl.h or
sysdeps/generic/include/fcntl.h? If it's the latter, I'd have to
create a sysdeps/hurd/include/fcntl.h that does not (re)define
O_IGNORE_CTTY (right?).

> > But this brings me to the more specific question: the headers to be
> > installed are also found using vpath during 'make install', right? How
> > would this work, will Make somehow know to not install this
> > sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep
> > installing io/fcntl.h?
>
> Afaiu there is no need to install any new header for this, this is an
> internal only definition to use on open call within glibc.

Yes, this is my point: you were suggesting that I add
sysdeps/unix/sysv/linux/fcntl.h (notice no /include/ in the middle),
which -- I'm not sure whether it would get installed by the build
system (instead of io/fcntl.h) or not. I want it to *not* get
installed. The option with /include/ in the middle that you're
suggesting now should take care of that.

Sergey

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

[Sergey Bugaev]

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

Re: [PATCH v3 1/2] include/fcntl.h: Define O_IGNORE_CTTY

[Adhemerval Zanella Netto]

On 13/06/23 06:42, Sergey Bugaev wrote:
> Hello,
> 
> On Mon, Jun 12, 2023 at 9:56 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>> Do you think the Linux port should define O_IGNORE_CTTY to O_NOCTTY
>>> and not to 0?
>>
>> Hurd O_IGNORE_CTTY and Linux O_NOCTTY do not have the *exactly* semantic,
>> so I think we should avoid change the internal open flags in this
>> specific patch.
> 
> OK.
> 
>> We have the internal header file, to say 'include/fcntl.h', which is
>> used when building glibc itself including the tests.  The internal-only
>> header also includes the installed one, 'io/fcntl.h', which defines
>> the ABI glibc provides.
> 
> Yes, that I understand.
> 
>> So to override a internal-only definition we have some options:
>>
>>   1. Override the file which is has precedence in the sysdeps selection
>>      (which defines the -I at built time).  So if you add the file
>>      "sysdeps/unix/sysv/linux/include/fcntl.h'., it would be included
>>      instead of 'include/fcntl.h'.
> 
> Oh cool, I didn't realize it was possible to nest include/ under
> sysdeps/, thank you! I thought there was only the top-level include/
> directory, but now I see that there are more, including even a few
> Mach/Hurd specific ones.
This comes from how the sysdeps (or sysnames from configuration) is organized
and used as include directories, it allows system specific path (which as
more 'deep' in sysdeps) to take precedence over default ones.

> 
>>   2. Add per-system file that is included in the generic 'include/fcntl.h',
>>      for instance fcntl-system.h or something like that.  On then add
>>      a generic definition on sysdep/generic/ with the expected value
>>      and override it on any sysdep folder that requires it.
> 
> Is there already an example of such a header file? I can't find any
> files in any include/ directory matching either *sysdep* or *system*.

Usually for system specific headers we do not add them on include/ but
rather on sysdeps/generic and override when necessary.  For instance
the sysdeps/generic/math_ldbl.h which is override multiple times
because of the multiple long double ABIs we support

Another example is the malloc-hugepages.c, which has a common definition
but the implementation is reimplemented for Linux.

> 
>> I tend to see the second options as a slight simpler one.
> 
> The first option sounds simpler to me.
> 
> Should it be sysdeps/unix/sysv/linux/include/fcntl.h or
> sysdeps/generic/include/fcntl.h? If it's the latter, I'd have to
> create a sysdeps/hurd/include/fcntl.h that does not (re)define
> O_IGNORE_CTTY (right?).

I think for this it would be better to just add a sysdeps/generic/fcntl-system.h
(or a better name if you a better idea), and define O_IGNORE_CTTY as 0.
The Hurd version will then have a different value.  Then include it on 
include/fcntl.h.

Btw, we already do it for dl-fcntl.h to handle another Hurd requirement.

> 
>>> But this brings me to the more specific question: the headers to be
>>> installed are also found using vpath during 'make install', right? How
>>> would this work, will Make somehow know to not install this
>>> sysdeps/unix/sysv/linux/fcntl.h file that you're proposing, and keep
>>> installing io/fcntl.h?
>>
>> Afaiu there is no need to install any new header for this, this is an
>> internal only definition to use on open call within glibc.
> 
> Yes, this is my point: you were suggesting that I add
> sysdeps/unix/sysv/linux/fcntl.h (notice no /include/ in the middle),
> which -- I'm not sure whether it would get installed by the build
> system (instead of io/fcntl.h) or not. I want it to *not* get
> installed. The option with /include/ in the middle that you're
> suggesting now should take care of that.

Afaik none of the header on include are installed, each subdir Makefile
needs to explicit add the expected file on 'headers' rule.  Also, for
sysdeps we have an extra rule, sysdep_headers, that adds extra includes.

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

[Paul Eggert]

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?

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

[Sergey Bugaev]

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

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

[Paul Eggert]

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.

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

[Samuel Thibault]

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

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

[Sergey Bugaev]

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