unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
* [PATCH 1/1] fport_print: handle ttyname ENODEV
@ 2025-01-17 19:18 Rob Browning
  2025-01-17 19:27 ` Rob Browning
  2025-01-19 12:52 ` Maxime Devos
  0 siblings, 2 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-17 19:18 UTC (permalink / raw)
  To: guile-devel

In some situations, ttyname may return ENODEV even though isatty is
true.  From ttyname(3):

  A process that keeps a file descriptor that refers to a pts(4) device
  open when switching to another mount namespace that uses a different
  /dev/ptmx instance may still accidentally find that a device path of
  the same name for that file descriptor exists.  However, this device
  path refers to a different device and thus can't be used to access the
  device that the file descriptor refers to.  Calling ttyname() or
  ttyname_r() on the file descriptor in the new mount namespace will
  cause these functions to return NULL and set errno to ENODEV.

Observed in a Debian riscv64 porterbox schroot.

When ttyname fails with ENODEV, just include the file descriptor integer
value instead.  Call ttyname() rather than scm_ttyname() to avoid some
extra work and having to catch the ENODEV.

* libguile/fports/c: include the integer fd when ttyname() fails with
ENODEV.
---

 Proposed for main.  (I'll probably include this in Debian
 immediately.)

 libguile/fports.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/libguile/fports.c b/libguile/fports.c
index 9d4ca6ace..267ed632f 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -554,6 +554,7 @@ SCM_DEFINE (scm_adjust_port_revealed_x, "adjust-port-revealed!", 2, 0, 0,
 
 
 \f
+#define FUNC_NAME "fport_print"
 static int 
 fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
 {
@@ -571,11 +572,36 @@ fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
       fdes = (SCM_FSTREAM (exp))->fdes;
 
 #if (defined HAVE_TTYNAME) && (defined HAVE_POSIX)
-      if (isatty (fdes))
-	scm_display (scm_ttyname (exp), port);
+      if (!isatty (fdes))
+	scm_intprint (fdes, 10, port);
       else
-#endif /* HAVE_TTYNAME */
+        {
+          scm_i_scm_pthread_mutex_lock (&scm_i_misc_mutex);
+
+          char *name;
+          SCM_SYSCALL (name = ttyname (fdes));
+          int err = errno;
+          if (name != NULL)
+            name = strdup (name);
+
+          scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
+
+          if (name)
+            scm_display (scm_take_locale_string (name), port);
+          else
+            {
+              if (err == ENODEV)
+                scm_intprint (fdes, 10, port);
+              else
+                {
+                  errno = err;
+                  SCM_SYSERROR;
+                }
+            }
+        }
+#else /* can't use ttyname */
 	scm_intprint (fdes, 10, port);
+#endif
     }
   else
     {
@@ -586,6 +612,7 @@ fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
   scm_putc ('>', port);
   return 1;
 }
+#undef FUNC_NAME
 
 /* fill a port's read-buffer with a single read.  returns the first
    char or EOF if end of file.  */
-- 
2.45.2




^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-17 19:18 [PATCH 1/1] fport_print: handle ttyname ENODEV Rob Browning
@ 2025-01-17 19:27 ` Rob Browning
  2025-01-19 12:52 ` Maxime Devos
  1 sibling, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-17 19:27 UTC (permalink / raw)
  To: guile-devel

Rob Browning <rlb@defaultvalue.org> writes:

> Call ttyname() rather than scm_ttyname() to avoid some
> extra work and having to catch the ENODEV.

I also wondered about using ttyname_r() when it's available, to avoid
locking, perhaps in a function shared by this and scm_ttyname(), but if
that's even advisable, we could always handle that separately.

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-17 19:18 [PATCH 1/1] fport_print: handle ttyname ENODEV Rob Browning
  2025-01-17 19:27 ` Rob Browning
@ 2025-01-19 12:52 ` Maxime Devos
  2025-01-19 16:51   ` Rob Browning
  1 sibling, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-19 12:52 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

You are ignoring out-of-memory situations (from strdup), instead of raising an out-of-memory exception.

It’s rather unclear what you are taking the scm_i_misc_mutex for.

You are also not taking the appropriate port lock (look for scm_i_port_table_mutex – savannah git is down and I don’t have a local copy for a more precise reference – you also need to take the per-fdes).  (the fdes = … in the beginning doesn’t need a lock, since the fdes is only printed, but if you act upon it you need a lock to ensure the fdes doesn’t move)

If you are doing strdup only to turn it into a Scheme string afterwards with scm_take_locale_string, you might as well do scm_from_locale_string directly  (Except that this may lead to an exception while a lock is being held which isn’t ideal, but this can happen anyway since SCM_SYSCALL can raise exceptions.)

The way you are handling the copy of the string here is rather risky – if the syscall results in EINTR, and an async is queued, then the async is run, and this async might cal ‘ttyname’, potentially clobbering the old ttyname. I’m not sure, but I think ttyname isn’t supposed to return EINTR. It has been a while since I’ve looked at Hurd RPCs, but I think the errno aren’t constrained to a fixed set of errno, so to some degree you need to deal with non-standard term servers (even if you don’t intend to deal with such servers normally, what if a malicious user communicates with a program of another (innocent) user, and part of that program involves communicating with potentially malicious user-provided ttys and term servers(*)?).  (Maybe the Hurd already eliminates EINTR somewhere, but that’s something you would need to verify.)

(*) not sure about standard Hurd terminology

ttyname is also thread-unsafe on the Hurd (https://github.com/bminor/glibc/blob/91bb902f58264a2fd50fbce8f39a9a290dd23706/sysdeps/mach/hurd/ttyname.c#L32). According to the glibc description, it is thread-unsafe in general. Maybe that’s what the mutex is for. But then, you are duplicating scm_ttyname (well, probably, I don’t have a local copy available). So, you could just use scm_ttyname and let scm_ttyname deal with all the locking, interrupts and thread-safety. If catching the system-error causes difficulty, you could  define a variant scm_i_ttyname that returns the string or the errno (which is in turn used by scm_ttyname), and use scm_i_ttyname.

Also, the explanation in the commit message belongs in a (code) comment, so people don’t have to track down the commit that introduced the changes.

Best regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 4315 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-19 12:52 ` Maxime Devos
@ 2025-01-19 16:51   ` Rob Browning
  2025-01-19 16:56     ` Rob Browning
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-19 16:51 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> You are ignoring out-of-memory situations (from strdup), instead of raising an out-of-memory exception.

Hmm, just to clarify.  *I* didn't write any of that code, and hadn't
considered it carefully yet -- I just replicated the existing
scm_ttyname() code from posix.c, i.e. this just does what we already do
there.

But of course, that may also warrant improvement.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-19 16:51   ` Rob Browning
@ 2025-01-19 16:56     ` Rob Browning
  2025-01-19 22:40       ` Rob Browning
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-19 16:56 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

> Hmm, just to clarify.  *I* didn't write any of that code, and hadn't
> considered it carefully yet -- I just replicated the existing
> scm_ttyname() code from posix.c, i.e. this just does what we already do
> there.

Oh, and in addition to wondering about using ttyname_r when it's
available, if we did that, or even if we didn't, I could also see moving
the shared code to a shared helper, but I was just starting with a
"simpler" proposal (to avoid the crash).

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-19 16:56     ` Rob Browning
@ 2025-01-19 22:40       ` Rob Browning
  2025-01-20 22:05         ` Rob Browning
  2025-01-21 16:36         ` Maxime Devos
  0 siblings, 2 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-19 22:40 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

Rob Browning <rlb@defaultvalue.org> writes:

> Oh, and in addition to wondering about using ttyname_r when it's
> available, if we did that, or even if we didn't, I could also see moving
> the shared code to a shared helper, but I was just starting with a
> "simpler" proposal (to avoid the crash).

Attached is a variant of the fancier approach (also relying on ttyname_r
when available) that I'd been thinking of (though might not be quite
right yet), and which I realized you probably also suggested.

The TTY_NAME_MAX comment's referring to whether or not "The maximum
length of the terminal name shall be {TTY_NAME_MAX}." here only applies
to the paragraph it's in (i.e. just for ttyname_r()), or applies to
ttyname() too:

  https://pubs.opengroup.org/onlinepubs/009696899/functions/ttyname.html


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: handle-enodev-in-fport-print.patch --]
[-- Type: text/x-diff, Size: 7446 bytes --]

From 94b338f144441483b0d0513be5c90d88c7aaeffb Mon Sep 17 00:00:00 2001
From: Rob Browning <rlb@defaultvalue.org>
Date: Fri, 17 Jan 2025 11:45:26 -0600
Subject: [PATCH 1/1] fport_print: handle ttyname ENODEV

In some situations, ttyname may return ENODEV even though isatty is
true.  From ttyname(3):

  A process that keeps a file descriptor that refers to a pts(4) device
  open when switching to another mount namespace that uses a different
  /dev/ptmx instance may still accidentally find that a device path of
  the same name for that file descriptor exists.  However, this device
  path refers to a different device and thus can't be used to access the
  device that the file descriptor refers to.  Calling ttyname() or
  ttyname_r() on the file descriptor in the new mount namespace will
  cause these functions to return NULL and set errno to ENODEV.

Observed in a Debian riscv64 porterbox schroot.

When ttyname fails with ENODEV, just include the file descriptor integer
value instead.  Call ttyname() rather than scm_ttyname() to avoid some
extra work and having to catch the ENODEV.

* libguile/fports.c: include the integer fd when ttyname() fails with
ENODEV.
* libguile/posix.c: add scm_i_c_ttyname().
* libguile/posix.h: add scm_i_c_ttyname().
---
 configure.ac      |  2 +-
 libguile/fports.c | 31 ++++++++++++++++---
 libguile/posix.c  | 78 ++++++++++++++++++++++++++++-------------------
 libguile/posix.h  |  1 +
 4 files changed, 74 insertions(+), 38 deletions(-)

diff --git a/configure.ac b/configure.ac
index af9176b46..2e89f93a9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -549,7 +549,7 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid         \
   nice readlink rmdir setegid seteuid                            \
   setuid setgid setpgid setsid sigaction siginterrupt stat64  \
   strptime symlink sync sysconf tcgetpgrp tcsetpgrp uname waitpid       \
-  strdup usleep on_exit chown link fcntl ttyname getpwent \
+  strdup usleep on_exit chown link fcntl ttyname ttyname_r getpwent \
   getgrent kill getppid getpgrp fork setitimer getitimer strchr strcmp  \
   index bcopy rindex truncate isblank _NSGetEnviron              \
   strcoll_l strtod_l strtol_l newlocale uselocale utimensat     \
diff --git a/libguile/fports.c b/libguile/fports.c
index 9d4ca6ace..5ba09c552 100644
--- a/libguile/fports.c
+++ b/libguile/fports.c
@@ -554,6 +554,7 @@ SCM_DEFINE (scm_adjust_port_revealed_x, "adjust-port-revealed!", 2, 0, 0,
 
 
 \f
+#define FUNC_NAME "fport_print"
 static int 
 fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
 {
@@ -570,12 +571,31 @@ fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
       scm_putc (' ', port);
       fdes = (SCM_FSTREAM (exp))->fdes;
 
-#if (defined HAVE_TTYNAME) && (defined HAVE_POSIX)
-      if (isatty (fdes))
-	scm_display (scm_ttyname (exp), port);
-      else
-#endif /* HAVE_TTYNAME */
+#if ((defined HAVE_TTYNAME) || (defined HAVE_TTYNAME_R)) && (defined HAVE_POSIX)
+      if (!isatty (fdes))
 	scm_intprint (fdes, 10, port);
+      else
+        {
+          SCM name = scm_i_c_ttyname (fdes);
+          if (scm_is_string (name))
+            scm_display (name, port);
+          else
+            {
+              const int err = scm_to_int (name);
+              // In some situations, ttyname may return ENODEV even
+              // though isatty is true.  cf. the GNU/Linux ttyname(3).
+              if (err == ENODEV)
+                scm_intprint (fdes, 10, port);
+              else
+                {
+                  errno = err;
+                  SCM_SYSERROR;
+                }
+            }
+        }
+#else /* can't use ttyname */
+      scm_intprint (fdes, 10, port);
+#endif
     }
   else
     {
@@ -586,6 +606,7 @@ fport_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED)
   scm_putc ('>', port);
   return 1;
 }
+#undef FUNC_NAME
 
 /* fill a port's read-buffer with a single read.  returns the first
    char or EOF if end of file.  */
diff --git a/libguile/posix.c b/libguile/posix.c
index 0e57f012b..a9f726f98 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1043,52 +1043,66 @@ SCM_DEFINE (scm_getsid, "getsid", 1, 0, 0,
 #endif /* HAVE_GETSID */
 
 
-/* ttyname returns its result in a single static buffer, hence
-   scm_i_misc_mutex for thread safety.  In glibc 2.3.2 two threads
-   continuously calling ttyname will otherwise get an overwrite quite
-   easily.
+#if (defined HAVE_TTYNAME) || (defined HAVE_TTYNAME_R)
 
-   ttyname_r (when available) could be used instead of scm_i_misc_mutex, but
-   there's probably little to be gained in either speed or parallelism.  */
+#define FUNC_NAME "scm_i_c_ttyname"
+SCM
+scm_i_c_ttyname (int fd)
+{
+  // Return the string ttyname or the integer errno.
+  int err = 0;
+  char name[TTY_NAME_MAX];
+
+#ifdef HAVE_TTYNAME_R
+  SCM_SYSCALL (err = ttyname_r (fd, name, TTY_NAME_MAX));
+#else // HAVE_TTYNAME
+  // ttyname returns its result in a single static buffer, hence
+  // scm_i_misc_mutex for thread safety.  In glibc 2.3.2 two threads
+  // continuously calling ttyname will otherwise get an overwrite quite
+  // easily.
+  scm_i_scm_pthread_mutex_lock (&scm_i_misc_mutex);
+  char *global_name;
+  SCM_SYSCALL (global_name = ttyname (fd));
+  err = errno;
+  if (!err)
+    {
+      // Not necessary if ttyname() must also respect TTY_NAME_MAX.
+      // POSIX ttyname description isn't completely clear.
+      if (strlen (global_name) > TTY_NAME_MAX - 1)
+        errno = ERANGE;
+      else
+        strcpy(name, global_name);
+    }
+  scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
+#endif // HAVE_TTYNAME
 
-#ifdef HAVE_TTYNAME
-SCM_DEFINE (scm_ttyname, "ttyname", 1, 0, 0, 
+  if (err)
+    return scm_from_int(err);
+  else
+    return scm_from_locale_string (name);
+}
+#undef FUNC_NAME
+
+SCM_DEFINE (scm_ttyname, "ttyname", 1, 0, 0,
             (SCM port),
 	    "Return a string with the name of the serial terminal device\n"
 	    "underlying @var{port}.")
 #define FUNC_NAME s_scm_ttyname
 {
-  char *result;
-  int fd, err;
-  SCM ret = SCM_BOOL_F;
-
   port = SCM_COERCE_OUTPORT (port);
   SCM_VALIDATE_OPPORT (1, port);
   if (!SCM_FPORTP (port))
     return SCM_BOOL_F;
-  fd = SCM_FPORT_FDES (port);
 
-  scm_i_scm_pthread_mutex_lock (&scm_i_misc_mutex);
-
-  SCM_SYSCALL (result = ttyname (fd));
-  err = errno;
-  if (result != NULL)
-    result = strdup (result);
-
-  scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
-
-  if (!result)
-    {
-      errno = err;
-      SCM_SYSERROR;
-    }
-  else
-    ret = scm_take_locale_string (result);
-
-  return ret;
+  SCM result = scm_i_c_ttyname (SCM_FPORT_FDES (port));
+  if (scm_is_string (result))
+    return result;
+  errno = scm_to_int (result);
+  SCM_SYSERROR;
 }
 #undef FUNC_NAME
-#endif /* HAVE_TTYNAME */
+
+#endif // (defined HAVE_TTYNAME) || (defined HAVE_TTYNAME_R)
 
 
 /* For thread safety "buf" is used instead of NULL for the ctermid static
diff --git a/libguile/posix.h b/libguile/posix.h
index a4b0297b3..acfaa3c70 100644
--- a/libguile/posix.h
+++ b/libguile/posix.h
@@ -91,6 +91,7 @@ SCM_API SCM scm_sethostname (SCM name);
 SCM_API SCM scm_gethostname (void);
 SCM_API SCM scm_getaffinity (SCM pid);
 SCM_API SCM scm_setaffinity (SCM pid, SCM cpu_set);
+SCM_INTERNAL SCM scm_i_c_ttyname (int fd);
 SCM_INTERNAL void scm_init_posix (void);
 
 SCM_INTERNAL scm_i_pthread_mutex_t scm_i_locale_mutex;
-- 
2.45.2


[-- Attachment #3: Type: text/plain, Size: 198 bytes --]


-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-19 22:40       ` Rob Browning
@ 2025-01-20 22:05         ` Rob Browning
  2025-01-21 16:36         ` Maxime Devos
  1 sibling, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-20 22:05 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

> +      // Not necessary if ttyname() must also respect TTY_NAME_MAX.
> +      // POSIX ttyname description isn't completely clear.
> +      if (strlen (global_name) > TTY_NAME_MAX - 1)
> +        errno = ERANGE;

Oops:

  err = ERANGE

...and perhaps a bit more care wrt bounds elsewhere.

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-19 22:40       ` Rob Browning
  2025-01-20 22:05         ` Rob Browning
@ 2025-01-21 16:36         ` Maxime Devos
  2025-01-21 18:28           ` Rob Browning
  1 sibling, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-21 16:36 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

>+      // Not necessary if ttyname() must also respect TTY_NAME_MAX.
>+      // POSIX ttyname description isn't completely clear.

Not necessary -> not certain. I don’t think the ‘#define FUNC_NAME "scm_i_c_ttyname"’ is actually doing something for scm_i_ttyname, given the absence of SCM_SYSERROR. But maybe it’s a consistency thing. I don’t recall ‘//’ comments being in use in Guile but maybe I just forgot. Since the internal function is only used in the same file as which it is defined in, you can make it a static function and remove it from posix.h.

And shouldn’t be the description of the function be right before the function declaration (and maybe before the #if too, dunno) instead of inside?

Also, the locking is like

lock()
do syscall and maybe raise exception (via asyncs in case of a weird EINTR(*), I don’t refer to system-error here)
unlock()

(*) see Hurd situation I mentioned.

You need to also ‘unlock’ in the case of an exception (maybe scm_i_scm_pthread_mutex_lock does so, but going by the name I wouldn’t expect it to.) I don’t know what the simplest way to do this, but the dynwind code could solve this.  Or, alternatively, block asyncs for a little while.

Simpler solution: all that SCM_SYSCALL does, if I remember correctly, is running the syscall in a loop and every iteration do a tick. But since EINTR is non-standard(IIUC), no loop is needed so we can just _not_ treat EINTR specially and raise an exception like in other cases (which happens outside the lock). And if you remove the EINTR handling, all you end up with is a simple syscall (and _maybe_ an async tick, which if kept could simply be moved after the unlock).

On length checks and the like: it isn’t clear to me whether TTY_NAME_MAX includes the terminating zero or not (I would expect not, like strlen). Although, ‘namesize’ _does_ include the terminating zero going by what’s written (*) (“The array is namesize characters long”, + see [ERANGE] description.).

(*) source: https://pubs.opengroup.org/onlinepubs/9699919799/, in particular ttyname.

(In case of unclarity and to be really sure to not copy too much, strncpy exists where n=length of array excluding space reserved for terminating zero, and things could be initialised to \0.)

Best regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 7609 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 16:36         ` Maxime Devos
@ 2025-01-21 18:28           ` Rob Browning
  2025-01-21 18:51             ` Rob Browning
  2025-01-21 19:41             ` Maxime Devos
  0 siblings, 2 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-21 18:28 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> I don’t think the ‘#define FUNC_NAME "scm_i_c_ttyname"’ is actually
> doing something for scm_i_ttyname, given the absence of SCM_SYSERROR.

Ahh, thanks.  With the rework, I don't need that anymore.

> Since the internal function is only used in the same file as which it
> is defined in, you can make it a static function and remove it from
> posix.h.

It's also used in fports.c in the patch -- we need the helper so we can
get the errno in order to handle ENODEV in fport_print.

> And shouldn’t be the description of the function be right before the
> function declaration (and maybe before the #if too, dunno) instead of
> inside?

In this case, I was just including it as an indication of the helper's
intent rather than "real" documentation.

> Also, the locking is like...

OK, so you're saying that the existing scm_ttyname() is potentially
broken in these ways.  Assuming so, then I might see about handling that
right now, but I think it could also be filed as a bug for later.

And depending on what you mean by "non-standard", I don't know whether
that change (e.g. to allow EINTR to propagate, and to stop processing
ticks) could go into a stable release.

In any case, I'm happy to fix all the problems if we can be confident
we're not causing a regression, and not introducing something
inappropriate for a Z release -- I'll probably look into it a bit
further today, but for now, my more immediate goal is to fix the test by
fixing the ENODEV problem, without making anything worse.

> On length checks and the like: it isn’t clear to me whether
> TTY_NAME_MAX includes the terminating zero or not (I would expect not,
> like strlen).

I wsn't sure at first either, but then I saw that POSIX says "The value
of namesize is smaller than the length of the string to be returned
including the terminating null character" for the ERANGE case, so I
assumed it was included:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/ttyname.html

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 18:28           ` Rob Browning
@ 2025-01-21 18:51             ` Rob Browning
  2025-01-21 19:03               ` Rob Browning
  2025-01-21 19:41             ` Maxime Devos
  1 sibling, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-21 18:51 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org


Regarding the EINTR, SCM_SYSCALL redirects to comments in scm_syserror
which suggest to me that this may be a known issue:

  https://git.savannah.gnu.org/cgit/guile.git/tree/libguile/error.c?h=v3.0.10&id=b2cc237a02dcb13625885e76df28bc254a522100#n135

If so, then I'm inclined to preserve the existing scm_ttyname() behavior
for now.

I'll look a bit more closely at your locking concern.  Unless there's
something else going on, I completely agree that we don't want to exit
without releasing the lock.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 18:51             ` Rob Browning
@ 2025-01-21 19:03               ` Rob Browning
  2025-01-21 21:03                 ` Maxime Devos
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-21 19:03 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

> I'll look a bit more closely at your locking concern.  Unless there's
> something else going on, I completely agree that we don't want to exit
> without releasing the lock.

Assuming you don't see anything amiss, perhaps I'll include this:

diff --git a/libguile/posix.c b/libguile/posix.c
index 14fefe183..dc0bbea29 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1059,7 +1059,8 @@ scm_i_c_ttyname (int fd)
   // scm_i_misc_mutex for thread safety.  In glibc 2.3.2 two threads
   // continuously calling ttyname will otherwise get an overwrite quite
   // easily.
-  scm_i_scm_pthread_mutex_lock (&scm_i_misc_mutex);
+  scm_dynwind_begin (0);
+  scm_i_dynwind_pthread_mutex_lock (&scm_i_misc_mutex);
   char *global_name;
   SCM_SYSCALL (global_name = ttyname (fd));
   err = errno;
@@ -1068,11 +1069,11 @@ scm_i_c_ttyname (int fd)
       // Not necessary if ttyname() must also respect TTY_NAME_MAX.
       // POSIX ttyname description isn't completely clear.
       if (strlen (global_name) > TTY_NAME_MAX - 1)
         err = ERANGE;
       else
         strcpy(name, global_name);
     }
-  scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
+  scm_dynwind_end ();
 #endif // HAVE_TTYNAME
 
   if (err)

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 18:28           ` Rob Browning
  2025-01-21 18:51             ` Rob Browning
@ 2025-01-21 19:41             ` Maxime Devos
  2025-01-21 20:04               ` Rob Browning
  1 sibling, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-21 19:41 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 4999 bytes --]

>>[…]
>It's also used in fports.c in the patch -- we need the helper so we can
get the errno in order to handle ENODEV in fport_print.

Ok, I think I assumed everything was being done in one file and glossed over the file names.

>> Also, the locking is like...

>OK, so you're saying that the _existing_ scm_ttyname() is potentially
broken in these ways.  Assuming so, then I might see about handling that
right now, but I think it could also be filed as a bug for later.

(emphasis added) To be clear, it’s the new version I looked at (unless I misread some + for - or something). But since the new is based on the old …

>And depending on what you mean by "non-standard", I don't know whether
that change (e.g. to allow EINTR to propagate, and to stop processing
ticks) could go into a stable release.

“non-standard”, as in:

• EINTR is not in the list of errors mentioned by POSIX (so would never happen on correct POSIX systems)
• There does not appear to be a compelling reason to deviate from POSIX.
• Even without POSIX stating it, it seems hard to find a good reason to return EINTR.
• Did you ever see (outside somewhat overzealous automation) any code that checks for EINTR? If you are implementing a kernel, better don’t return EINTR, or it would break things.
• I don’t know _any_ system that has ttyname return EINTR (Hurd theoretically can, but that’s for custom POSIX-violating term servers making rather … dubious choices).

(You could still retry after EINTR, but you would need to implement a loop yourself: something like do { lock(); syscall(); save errno; unlock(); tick} while(error == EINTR) }.)

On ticks: it’s not that ticks stop being processed, rather (assuming you move the async tick outside the locking/unlocking but still keep it in the function) that they won’t be processed right next to the syscall.

Also, while I wouldn’t recommend it, it probably wouldn’t cause issues in the wild to remove the async tick entirely for scm_ttyname (or scm_c_i_ttyname, whatever) - unless there is code in the wild that (1) is written in C (the Scheme equivalent isn’t affected because the Scheme compiler automatically inserts async ticks (*) in case of loop constructs) and (2) does something like:

// lots of iterations, doesn’t have to be actually infinite
while(1) {
  scm_ttyname(something)
  // and don’t call any I/O function here,
  // and don’t call any other Guile syscall-like function that likely do an (possibly indirect) async tick
 // (also some other Guile functions may have safe points)
 // and don’t call any interesting Scheme procedures (if (*) is true, don’t call any Scheme procedure at all)
 // and don’t a manual SCM_ASYNC_TICK (or was it SCM_TICK?)
}
// ^ why would you even do this???

(*) I think it also does this for procedure calls but I’m not sure.

(To be clear, I’m only saying this about ttyname. There likely are many other syscalls where you can shuffle the safepoint/tick around a little, but I’m not making a general claim here.) 

>In any case, I'm happy to fix all the problems if we can be confident
we're not causing a regression, and not introducing something
inappropriate for a Z release -- I'll probably look into it a bit
further today, but for now, my more immediate goal is to fix the test by
fixing the ENODEV problem, without making anything worse.

I, for one, am confident that the proposed async tick adjustments for ttyname does not cause a regression.

I’m not really confident about what’s all going on with buffer sizes and maximum length, and even if the interpretation of POSIX used here is _correct_, it might be the case that the OS interpretation is wrong and we  need to deal with it.

(Perhaps looking at the Linux kernel side may be a good check?)

> On length checks and the like: it isn’t clear to me whether
> TTY_NAME_MAX includes the terminating zero or not (I would expect not,
> like strlen).

>I wsn't sure at first either, but then I saw that POSIX says "The value
of namesize is smaller than the length of the string to be returned
including the terminating null character" for the ERANGE case, so I
assumed it was included:
>https://pubs.opengroup.org/onlinepubs/9699919799/functions/ttyname.html

True, but that’s ‘namesize’, not a hypothetical ‘namelength’. And the description in the beginning says “the maximum _length_ of the terminal name shall be {TTY_NAME_MAX}”. But is that the specified behaviour or is it sloppy writing?

If only POSIX said either ‘(excluding terminating zero)’ or ‘(including terminating null character)’, that would have made things clear, but no …

All that said, perhaps just use ‘TTY_NAME_MAX+1’ instead of ‘TTY_NAME_MAX’? It’s not like we care here about not exceeding the nominal maximum for its own sake, rather it’s just to have a sufficiently large buffer.

Best regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 13403 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 19:41             ` Maxime Devos
@ 2025-01-21 20:04               ` Rob Browning
  2025-01-21 20:49                 ` Maxime Devos
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-21 20:04 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> But since the new is based on the old …

Right.  I'm most concerned with the changes being made relative to what
we had.

> • EINTR is not in the list of errors mentioned by POSIX (so would never happen on correct POSIX systems)

I'm not sure this matters for a Z release.  Current Guile will handle
EINTR for ttyname() right now (the way it does for everything else), and
changing it might break any systems that, correctly or incorrectly
according to POSIX, do return EINTR.

> • Did you ever see (outside somewhat overzealous automation) any code
> that checks for EINTR? If you are implementing a kernel, better don’t
> return EINTR, or it would break things.

Clearly Guile does all the time via (at least) SCM_SYSCALL?  In any
case, also related
https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

And I'm pretty sure I have seen EINTR before myself, but I haven't been
doing the relevant C-level work intensively for a while.

> (You could still retry after EINTR, but you would need to implement a loop yourself: something like do { lock(); syscall(); save errno; unlock(); tick} while(error == EINTR) }.)

I don't understand -- Guile clearly intends to do a specific thing right
now for EINTR (described by SCM_SYSCALL and scm_syserror).  Why should
ttyname() be treated specially?

And I feel like whether or not that specific approach should be
reconsidered is outside the scope of this change.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 20:04               ` Rob Browning
@ 2025-01-21 20:49                 ` Maxime Devos
  2025-01-21 21:56                   ` Rob Browning
  0 siblings, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-21 20:49 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

>> • EINTR is not in the list of errors mentioned by POSIX (so would never happen on correct POSIX systems)

>I'm not sure this matters for a Z release.  Current Guile will handle
EINTR for ttyname() right now (the way it does for everything else), and
changing it might break any systems that, correctly or incorrectly
according to POSIX, do return EINTR.

It’s the combination of all the bullet points above. It’s an ‘X, and Y, and Z’ list, not an ‘X, or Y, or Z’ list.

Handling EINTR for ttyname is pointless if ttyname never produces EINTR and nobody is interested in writing a ttyname that produces EINTR.

>> • Did you ever see (outside somewhat overzealous automation) any code
>> that checks for EINTR? If you are implementing a kernel, better don’t
>> return EINTR, or it would break things.

>Clearly Guile does all the time via (at least) SCM_SYSCALL?

(As you appear to be consider syscalls in general, not ttyname in particular) No. For example, it doesn’t for the following:

• open_or_open64: https://git.savannah.gnu.org/cgit/guile.git/tree/libguile/filesys.c#n1331
• lseek_or_lseek64:  https://git.savannah.gnu.org/cgit/guile.git/tree/libguile/filesys.c#n1454
• getcwd: https://git.savannah.gnu.org/cgit/guile.git/tree/libguile/filesys.c#n1507

The SCM_SYSCALL around the ttyname, I consider to be a case of overzealous automation – the SCM_SYSCALL is the automation, and the writer adding it around the overzealous.

> In any
case, also related
>https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
>And I'm pretty sure I have seen EINTR before myself, but I haven't been
doing the relevant C-level work intensively for a while.

Those bullets points are for ‘ttyname’ in particular, not syscalls in general.
There are plenty of places where EINTR is to be expected (e.g. write and read), but ttyname isn’t one of them. 

>> (You could still retry after EINTR, but you would need to implement a loop yourself: something like do { lock(); syscall(); save errno; unlock(); tick} while(error == EINTR) }.)

>I don't understand -- Guile clearly intends to do a specific thing right
now for EINTR (described by SCM_SYSCALL and scm_syserror).  Why should
ttyname() be treated specially?

I didn’t say anywhere that ttyname is to be treated specially. If other functions have the same issue with locking and exceptions from asyncs and simply (incorrectly) use SCM_SYSERROR, they would need similar adjustments.

Also, I highly doubt that Guile intends for the unlocking to not happen. Even if it did intend that, then it intends wrong and its intentions should be changed.

It’s simply the case that SCM_SYSCALL is slightly the wrong tool here, so a slight variant is needed.

The reason for replacing the SCM_SYSCALL(… ttyname …) with a custom loop, is because of the locks. See: my previous e-mails on how the current version of the lock+unlock around the SCM_SYSCALL(…) is wrong. To correct that wrongness, moving the lock+unlock _inside_ the SCM_SYSCALL loop is a simple solution, and a solution that preserves handling EINTR.

If you suspect there are other cases where this pattern is needed, you could define a macro for it (maybe SCM_SYSCALL(pre, post, [insert  err=the syscall(…) here]), with in this particular case pre=lock(), post=unlock()) and put it right next to SCM_SYSCALL and its other variants.

>And I feel like whether or not that specific approach should be
reconsidered is outside the scope of this change.

How is fixing ttyname outside the scope of fixing ttyname? Previously, you said that:
 
>[context: ttyname]
>In any case, I'm happy to fix all the problems if we can be confident […]

Best  regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 11082 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 19:03               ` Rob Browning
@ 2025-01-21 21:03                 ` Maxime Devos
  0 siblings, 0 replies; 25+ messages in thread
From: Maxime Devos @ 2025-01-21 21:03 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

>Assuming you don't see anything amiss, perhaps I'll include this: [patch]

While I’m not familiar with how ‘dynwind’ is to be used from C, if the dynwind means what I’d expect it to mean, then this would Ensure the mutex is unlocked.

But, it has another problem: deadlocking, for some uses of asyncs.

• (Assuming the mutex is non-reentrant, and no async blocking) Suppose there is a queued async. Since the SCM_SYSCALL is between the locks, then the async is run with the misc mutex locked. Now, this async may choose to call ttyname, so now it’s taking the misc lock from _inside_ the misc lock. Which won’t work, since the mutex is non-reentrant. (The async doesn’t need to call ttyname per se, calling anything that also locks scm_i_misc_mutex_lock would be sufficient.)
• (Reentrant mutex case, less realistic) The async could very well send some message to another thread and wait for a response. If that other, to process that message, invokes ttyname (or, again, another procedure taking the lock), you have a deadlock situation.
• (More plausible situation, but a long wait instead of a real deadlock): The async needoes some intense calculations, or does something else that simply takes a long while. During this time, the scm_i_misc_mutex is locked, so other threads that want to use ttyname can’t progress for a long duration!

The variant of the SCM_SYSCALL loop I provided, avoids these issues.

Best regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 5319 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 20:49                 ` Maxime Devos
@ 2025-01-21 21:56                   ` Rob Browning
  2025-01-21 22:04                     ` Rob Browning
  2025-01-21 22:07                     ` Maxime Devos
  0 siblings, 2 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-21 21:56 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> It’s simply the case that SCM_SYSCALL is slightly the wrong tool here,
> so a slight variant is needed.

Good point.  How about this:

  SCM
  scm_i_c_ttyname (int fd)
  {
    // Return the string ttyname or the integer errno.  We can remove the
    // + 1 if we become confident POSIX requires TTY_NAME_MAX to include
    // the trailing null.  Assumes TTY_NAME_MAX < SIZE_MAX.
    int err = 0;
    char name[TTY_NAME_MAX + 1];

  #ifdef HAVE_TTYNAME_R
    SCM_SYSCALL (err = ttyname_r (fd, name, TTY_NAME_MAX));
  #else // HAVE_TTYNAME
    char *global_name;
    while(1)
      {
        // ttyname() may use a shared global buffer
        scm_i_pthread_mutex_lock (&scm_i_misc_mutex);
        global_name = ttyname (fd);
        err = errno;
        scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
        if (global_name || err != EINTR)
          break;
        scm_async_tick ();
      }
    strcpy(name, global_name);
  #endif // HAVE_TTYNAME

    if (err)
      return scm_from_int(err);
    else
      return scm_from_locale_string (name);
  }

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 21:56                   ` Rob Browning
@ 2025-01-21 22:04                     ` Rob Browning
  2025-01-21 22:07                     ` Maxime Devos
  1 sibling, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-21 22:04 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

>     while(1)
>       {
>         // ttyname() may use a shared global buffer
>         scm_i_pthread_mutex_lock (&scm_i_misc_mutex);
>         global_name = ttyname (fd);
>         err = errno;
>         scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
>         if (global_name || err != EINTR)
>           break;
>         scm_async_tick ();
>       }
>     strcpy(name, global_name);

Oops:

  if (global_name)
    strcpy (name, global_name);

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 21:56                   ` Rob Browning
  2025-01-21 22:04                     ` Rob Browning
@ 2025-01-21 22:07                     ` Maxime Devos
  2025-01-21 22:15                       ` Rob Browning
  1 sibling, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-21 22:07 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

> It’s simply the case that SCM_SYSCALL is slightly the wrong tool here,
> so a slight variant is needed.

>Good point.  How about this: […]

That’s it.

I would add a comment “// we don’t use SCM_SYSCALL, so we can keep the async tick outside the lock” (or worded differently ofc).

Best regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 1594 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 22:07                     ` Maxime Devos
@ 2025-01-21 22:15                       ` Rob Browning
  2025-01-21 23:05                         ` Rob Browning
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Browning @ 2025-01-21 22:15 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> That’s it.
>
> I would add a comment “// we don’t use SCM_SYSCALL, so we can keep the
> async tick outside the lock” (or worded differently ofc).

Plausible, though as a result of this I'm now a bit wary of SCM_SYSCALL.
If I get time, I might poke around a bit and see if the thing we've
attempted to fix here might be more widespread.

Thanks again
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 22:15                       ` Rob Browning
@ 2025-01-21 23:05                         ` Rob Browning
  2025-01-22  0:20                           ` Rob Browning
                                             ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-21 23:05 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

> If I get time, I might poke around a bit and see if the thing we've
> attempted to fix here might be more widespread.

I suppose something like this might serve as a generalization:

  #define SCM_LOCKED_SYSCALL(lock, body)                     \
    while(1)                                                 \
      {                                                      \
        scm_i_pthread_mutex_t *lock___ = (lock);             \
        scm_i_pthread_mutex_lock (lock___);                  \
        errno = 0;                                           \
        { body; }                                            \
        int err___ = errno;                                  \
        scm_i_pthread_mutex_unlock (lock___);                \
        if (err___ != EINTR)                                 \
          break;                                             \
        scm_async_tick ();                                   \
      }

Though a good, shorter name would be nice.  Then we'd have:

  #ifdef HAVE_TTYNAME_R
    SCM_SYSCALL (err = ttyname_r (fd, name, TTY_NAME_MAX));
  #else // HAVE_TTYNAME
    char *global_name = 0;
    SCM_LOCKED_SYSCALL(&scm_i_misc_mutex, global_name = ttyname (fd));
    if (global_name)
      strcpy(name, global_name);
  #endif // HAVE_TTYNAME
  
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 23:05                         ` Rob Browning
@ 2025-01-22  0:20                           ` Rob Browning
  2025-01-22 10:24                           ` Maxime Devos
  2025-01-22 19:23                           ` Rob Browning
  2 siblings, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-22  0:20 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

>         if (err___ != EINTR)                                 \
>           break;                                             \

Rather:

      if (err___ != EINTR)                                           \
        {                                                            \
          errno = err___;                                            \
          break;                                                     \
        }                                                            \

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 23:05                         ` Rob Browning
  2025-01-22  0:20                           ` Rob Browning
@ 2025-01-22 10:24                           ` Maxime Devos
  2025-01-23 18:55                             ` Rob Browning
  2025-01-22 19:23                           ` Rob Browning
  2 siblings, 1 reply; 25+ messages in thread
From: Maxime Devos @ 2025-01-22 10:24 UTC (permalink / raw)
  To: Rob Browning, guile-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

>I suppose something like this might serve as a generalization: […]

I’ve seen do { body; } while (0) in some places (albeit not in SCM_SYSCALL), which you might want investigate and maybe copy (instead of the { body; }), but I don’t know what the purpose of that construct is.

Otherwise (with the errno=… change mentioned in your other e-mail), it seems good to me.

>  [...], Then we'd have:
>    SCM_LOCKED_SYSCALL(&scm_i_misc_mutex, global_name = ttyname (fd));

Even if that were the only use of SCM_LOCKED_SYSCALL, it seems clearer that way (and less asymmetry).

I don’t think it can be shortened: it’s for Guile (so a SCM_ prefix for namespacing), it’s for syscalls (so _SYSCALL suffix), and it’s a variant for locking/unlocking (so LOCKED in the middle). Nothing to remove there, I’d think.

Best Regards,
Maxime Devos

[-- Attachment #2: Type: text/html, Size: 2671 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
@ 2025-01-22 15:43 dsmich
  0 siblings, 0 replies; 25+ messages in thread
From: dsmich @ 2025-01-22 15:43 UTC (permalink / raw)
  To: 'Maxime Devos'
  Cc: 'Rob Browning', 'guile-devel@gnu.org'

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

From: "Maxime Devos" 

	> I’ve seen do { body; } while (0) in some places (albeit not in
SCM_SYSCALL), which you might want investigate and maybe copy (instead
of the { body; }), but I don’t know what the purpose of that
construct is.It's a way of turning a block into a statement. So you
can use it in an if statement like:

#define MACRO() do { ... } while (0)
if (...) MACRO(); else ....

If the macro expanded to a block, it would make everyone unhappy.

-Dale


[-- Attachment #2: Type: text/html, Size: 668 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-21 23:05                         ` Rob Browning
  2025-01-22  0:20                           ` Rob Browning
  2025-01-22 10:24                           ` Maxime Devos
@ 2025-01-22 19:23                           ` Rob Browning
  2 siblings, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-22 19:23 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Rob Browning <rlb@defaultvalue.org> writes:

>     SCM_LOCKED_SYSCALL(&scm_i_misc_mutex, global_name = ttyname (fd));

Forgot to put the strcpy inside the SCM_LOCKED_SYSCALL...

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH 1/1] fport_print: handle ttyname ENODEV
  2025-01-22 10:24                           ` Maxime Devos
@ 2025-01-23 18:55                             ` Rob Browning
  0 siblings, 0 replies; 25+ messages in thread
From: Rob Browning @ 2025-01-23 18:55 UTC (permalink / raw)
  To: Maxime Devos, guile-devel@gnu.org

Maxime Devos <maximedevos@telenet.be> writes:

> Otherwise (with the errno=… change mentioned in your other e-mail), it seems good to me.

Thanks again for the review.  Broke it up into two commits, fixed a
couple of minor issues, and pushed it to main.

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2025-01-23 18:55 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-17 19:18 [PATCH 1/1] fport_print: handle ttyname ENODEV Rob Browning
2025-01-17 19:27 ` Rob Browning
2025-01-19 12:52 ` Maxime Devos
2025-01-19 16:51   ` Rob Browning
2025-01-19 16:56     ` Rob Browning
2025-01-19 22:40       ` Rob Browning
2025-01-20 22:05         ` Rob Browning
2025-01-21 16:36         ` Maxime Devos
2025-01-21 18:28           ` Rob Browning
2025-01-21 18:51             ` Rob Browning
2025-01-21 19:03               ` Rob Browning
2025-01-21 21:03                 ` Maxime Devos
2025-01-21 19:41             ` Maxime Devos
2025-01-21 20:04               ` Rob Browning
2025-01-21 20:49                 ` Maxime Devos
2025-01-21 21:56                   ` Rob Browning
2025-01-21 22:04                     ` Rob Browning
2025-01-21 22:07                     ` Maxime Devos
2025-01-21 22:15                       ` Rob Browning
2025-01-21 23:05                         ` Rob Browning
2025-01-22  0:20                           ` Rob Browning
2025-01-22 10:24                           ` Maxime Devos
2025-01-23 18:55                             ` Rob Browning
2025-01-22 19:23                           ` Rob Browning
  -- strict thread matches above, loose matches on Subject: below --
2025-01-22 15:43 dsmich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).