RE: [PATCH 1/1] fport_print: handle ttyname ENODEV

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