On Thu, Aug 08, 2024 at 03:04:29PM +0200, Theo Buehler wrote:
> 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.

Yes, it's exactly that commit what caused the issue.  I reported the bug
months ago on github.  I'd mentioned the issue in ports@:

  https://marc.info/?l=openbsd-ports&m=171790611818576&w=2

More late in misc@ I posted the links to my bug report:

  https://marc.info/?l=openbsd-misc&m=171947479126184&w=2

... but I realized after that that my github report isn't visible unless
you're logged in.  I already told them that I downloaded mpv sources and
reverting that commit fixed mpv under OpenBSD but as I already explained
here, they answered me that mpv developers don't use OpenBSD.  Finnaly
someone nicknamed N-R-K@ (the author of the diff that caused the issue)
said he'd run OpenBSD on qemu and take a look.  This was at Jul 2.
Still waiting. :-)



> 
> 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);
> + 
> 
> 

-- 
Walter