fix syscon fatal exit on ^Z

[sthalik]

I'm unsure what's the right behavior here. Maybe we should raise SIGSTOP in the handler? The current behavior of a fatal exit is hardly sensible however.

[sthalik]

Then again the least intrusive change would be to leave the signal handlers alone and just check for EINTR in addition to EAGAIN.

[skullernet]

EINTR checks don't look correct. Non-blocking read() or write() should never with EINTR. How can I reproduce this?

[sthalik]

^Z then bg the running process. Use stty tostop in the terminal and then stty -tostop. It's gonna trigger on at least either of these cases.

Note to self: it's enough to add a check for reading, writing is ok.

[sthalik]

Here's a repro from WSL. The issue was originally reported on a real Linux machine with the musl libc.

ananke /root/dev/q2pro # sudo -nunobody ./q2proded +map q2dm1                                          
[...]
q2proded r1711~0914b63, Sep 13 2018
https://github.com/skullernet/q2pro

sthalik connected
sthalik[127.0.0.1:58532]: q2pro r1707~14afeca Sep 11 2018 Win32 x86
sthalik entered the game
]
zsh: suspended  sudo -nunobody ./q2proded +map q2dm1
<148> ananke /root/dev/q2pro # bg
[1]  + continued  sudo -nunobody ./q2proded +map q2dm1
ananke /root/dev/q2pro #
ananke /root/dev/q2pro #
ananke /root/dev/q2pro # ********************
FATAL: tty_fatal_error: read() failed: Input/output error
********************

[1]  + exit 1     sudo -nunobody ./q2proded +map q2dm1
ananke /root/dev/q2pro # uname -a
Linux ananke 4.4.0-17134-Microsoft #137-Microsoft Thu Jun 14 18:46:00 PST 2018 x86_64 GNU/Linux

[skullernet]

Yes, but that causes read() to fail with EIO, not EINTR. Why are you re-adding EINTR checks? They were deliberately removed in commit 9c2446a because "interrupted" non-blocking calls don't make sense.

What is dummy signal handler for? Isn't checking for EIO and ignoring it enough? (I don't think pausing network-interactive application such as Q2PRO is correct behavior here.)

[sthalik]

What is dummy signal handler for? Isn't checking for EIO and ignoring it enough?

It's to get a consistent errno regardless off stty tostop or stty -tostop. See the blog post I referred to in the commit message.

I don't think pausing network-interactive application such as Q2PRO is correct behavior here.

It's not as much pausing, but having the controlling terminal return to the shell from the q2proded process. An example of this "controlling terminal" behavior on FreeBSD:

planetfall /root # cat &
[1] 8145
planetfall /root #
[1]  + suspended (tty input)  cat

[edit]

They were deliberately removed in commit 9c2446a because "interrupted" non-blocking calls don't make sense.

I fully agree, but that's what the tty read returns on Linux. WSL even reproduces this braindead behavior.

[skullernet]

It's to get a consistent errno regardless off stty tostop or stty -tostop. See the blog post I referred to in the commit message.

Can you explain? What kind of inconsistency? If the process is ignoring TSIGTTOU, then behavior should be identical according to the blog post you linked.

I don't think pausing network-interactive application such as Q2PRO is correct behavior here.

It's not as much pausing, but having the controlling terminal return to the shell from the q2proded process.

I meant that as a response to your suggestion to raise SIGSTOP in the handler.

What exactly is the goal of this PR? To fix fatal error on read() when using ^Z, or implement some specific behavior regarding to handling control of the terminal? If the latter, keep in mind that once Q2PRO initializes TTY support, the terminal is no longer usable by other programs anyway (it switches to non-blocking mode which many programs don't handle). Correct way to run Q2PRO within the same login session as other programs is to use a terminal multiplexer such as screen(1) or tmux(1).

[sthalik]

What exactly is the goal of this PR? To fix fatal error on read() when using ^Z, or implement some specific [...]

The former. Fix braindead EINTR from a non-blocking file descriptor. Maybe it could be reduced to a single EINTR check in the read handler?

Correct way to run Q2PRO within the same login session as other programs is to use a terminal multiplexer such as screen(1) or tmux(1).

Agreed, it's the kosher way. But it's a matter of robustness.

[skullernet]

The former. Fix braindead EINTR from a non-blocking file descriptor. Maybe it could be reduced to a single EINTR check in the read handler?

But EINTR never happens when the process is ignoring SIGTTIN/SIGTTOU. See here and here. It's pointless to check for EINTR. The following patch, however, works for me on Linux and FreeBSD:

--- a/src/unix/tty.c
+++ b/src/unix/tty.c
@@ -356,7 +356,7 @@ void Sys_RunConsole(void)
     tty_io->canread = false;
 
     if (ret < 0) {
-        if (errno == EAGAIN) {
+        if (errno == EAGAIN || errno == EIO) {
             return;
         }
         tty_fatal_error("read");

Can you fix the PR to contain only this change?

[sthalik]

Works on my end as well! Rebased against the master branch.

[skullernet]

Applied, thanks.