From: Theo Buehler <tb@theobuehler.org>
Subject: Re: changes to signal handling with respect to ksh ?
To: ports@openbsd.org
Date: Thu, 8 Aug 2024 15:04:29 +0200

On Thu, Aug 08, 2024 at 02:51:44PM +0200, Claudio Jeker wrote:
> On Thu, Aug 08, 2024 at 02:37:39PM +0200, Marc Espie wrote:
> > I've got several scripts that use mpv to display pictures.
> > 
> > It used to be that I could ^Z and fg on those scripts without any issues.
> > 
> > For a few weeks/months now, it seems to be broken. I have zero idea if
> > this is an issue with mpv, ksh, or signal handling.
> > 
> > I think mpv gets into another process group for whatever reason ?
> > 
> > It still gets suspended, but TCONT doesn't do a thing.
> > 
> > Rings a bell ?
> 
> I did a change on 2024/07/29 that affects SIGCONT but that is more recent
> than 'a few weeks/months now'. Doubt it is that but maybe check with the
> ftp.hostserver.de archive.

I think it's unrelated to claudio's commit. Upstream tried to fix a
race in 0.38 (mpv was updated in ports in May):

https://github.com/mpv-player/mpv/commit/b75b56f91048f0ca8f663b93a92aa059787022ce

Neither version of the signal handler looks great, but with that commit
reverted mpv seems to behave itself.

Index: patches/patch-osdep_terminal-unix_c
===================================================================
RCS file: patches/patch-osdep_terminal-unix_c
diff -N patches/patch-osdep_terminal-unix_c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ patches/patch-osdep_terminal-unix_c	8 Aug 2024 12:59:21 -0000
@@ -0,0 +1,65 @@
+Revert https://github.com/mpv-player/mpv/commit/b75b56f91048f0ca8f663b93a92aa059787022ce
+
+Index: osdep/terminal-unix.c
+--- osdep/terminal-unix.c.orig
++++ osdep/terminal-unix.c
+@@ -354,14 +354,29 @@ static int death_pipe[2] = {-1, -1};
+ enum { PIPE_STOP, PIPE_CONT };
+ static int stop_cont_pipe[2] = {-1, -1};
+ 
+-static void stop_cont_sighandler(int signum)
++static void stop_sighandler(int signum)
+ {
+     int saved_errno = errno;
+-    char sig = signum == SIGCONT ? PIPE_CONT : PIPE_STOP;
+-    (void)write(stop_cont_pipe[1], &sig, 1);
++    (void)write(stop_cont_pipe[1], &(char){PIPE_STOP}, 1);
++    (void)write(STDERR_FILENO, TERM_ESC_RESTORE_CURSOR,
++                sizeof(TERM_ESC_RESTORE_CURSOR) - 1);
+     errno = saved_errno;
++
++    // note: for this signal, we use SA_RESETHAND but do NOT mask signals
++    // so this will invoke the default handler
++    raise(SIGTSTP);
+ }
+ 
++static void continue_sighandler(int signum)
++{
++    int saved_errno = errno;
++    // SA_RESETHAND has reset SIGTSTP, so we need to restore it here
++    setsigaction(SIGTSTP, stop_sighandler, SA_RESETHAND, false);
++
++    (void)write(stop_cont_pipe[1], &(char){PIPE_CONT}, 1);
++    errno = saved_errno;
++}
++
+ static void safe_close(int *p)
+ {
+     if (*p >= 0)
+@@ -424,15 +439,6 @@ static MP_THREAD_VOID terminal_thread(void *ptr)
+             (void)read(stop_cont_pipe[0], &c, 1);
+             if (c == PIPE_STOP) {
+                 do_deactivate_getch2();
+-                if (isatty(STDERR_FILENO)) {
+-                    (void)write(STDERR_FILENO, TERM_ESC_RESTORE_CURSOR,
+-                                sizeof(TERM_ESC_RESTORE_CURSOR) - 1);
+-                }
+-                // trying to reset SIGTSTP handler to default and raise it will
+-                // result in a race and there's no other way to invoke the
+-                // default handler. so just invoke SIGSTOP since it's
+-                // effectively the same thing.
+-                raise(SIGSTOP);
+             } else if (c == PIPE_CONT) {
+                 getch2_poll();
+             }
+@@ -562,8 +568,8 @@ void terminal_init(void)
+     tcgetattr(tty_in, &tio_orig);
+ 
+     // handlers to fix terminal settings
+-    setsigaction(SIGCONT, stop_cont_sighandler, 0, true);
+-    setsigaction(SIGTSTP, stop_cont_sighandler, 0, true);
++    setsigaction(SIGCONT, continue_sighandler, 0, true);
++    setsigaction(SIGTSTP, stop_sighandler, SA_RESETHAND, false);
+     setsigaction(SIGTTIN, SIG_IGN, 0, true);
+     setsigaction(SIGTTOU, SIG_IGN, 0, true);
+