>> • 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
