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
