Hi Tim, trondd wrote on Wed, Sep 01, 2021 at 08:46:28PM -0400: > Ingo Schwarze <schwa...@usta.de> wrote: >> Ingo Schwarze wrote on Wed, Sep 01, 2021 at 04:38:51PM +0200:
>>> Note that the h_hup() and h_term() signal handlers are still unsafe >>> after this commit because they also set the "killersig" (how fitting!) >>> field in a global struct. >> I like it when fixing two bugs only amounts to minus: minus 14 LOC, >> 1 function, 1 global variable, 2 automatic variables, 1 struct member, >> 9 pointer dereferences, 1 #define, and minus 1 #undef. >> >> The argument that only one single GS exists applies unchanged. > Great. This is the direction I was going to go in with it. I hadn't > thought of collapsing h_hup an h_term, though. > > This makes sense as I understand the signal/event handling and a quick > test through the signals shows no difference in behavior. Thank you (and martijn@) for reviewing and testing. This is now committed. > Should h_term() and cl_sigterm be named something more general to > indicate that they also handle SIGHUP? Or is it good enough since it > includes all the signals that 'term'inate vi? It's not hard to follow > as-is. That is what i was thinking: why re-paint a bike shed if the existing paint is already pretty and not yet falling off? Yours, Ingo >> Index: cl/cl.h >> =================================================================== >> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v >> retrieving revision 1.11 >> diff -u -p -r1.11 cl.h >> --- cl/cl.h 1 Sep 2021 14:28:15 -0000 1.11 >> +++ cl/cl.h 1 Sep 2021 17:15:34 -0000 >> @@ -11,7 +11,6 @@ >> * @(#)cl.h 10.19 (Berkeley) 9/24/96 >> */ >> >> -extern volatile sig_atomic_t cl_sighup; >> extern volatile sig_atomic_t cl_sigint; >> extern volatile sig_atomic_t cl_sigterm; >> extern volatile sig_atomic_t cl_sigwinch; >> @@ -31,7 +30,6 @@ typedef struct _cl_private { >> char *rmso, *smso; /* Inverse video terminal strings. */ >> char *smcup, *rmcup; /* Terminal start/stop strings. */ >> >> - int killersig; /* Killer signal. */ >> #define INDX_HUP 0 >> #define INDX_INT 1 >> #define INDX_TERM 2 >> Index: cl/cl_funcs.c >> =================================================================== >> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v >> retrieving revision 1.21 >> diff -u -p -r1.21 cl_funcs.c >> --- cl/cl_funcs.c 1 Sep 2021 14:28:15 -0000 1.21 >> +++ cl/cl_funcs.c 1 Sep 2021 17:15:34 -0000 >> @@ -437,7 +437,7 @@ cl_refresh(SCR *sp, int repaint) >> * If we received a killer signal, we're done, there's no point >> * in refreshing the screen. >> */ >> - if (clp->killersig) >> + if (cl_sigterm) >> return (0); >> >> /* >> @@ -582,7 +582,7 @@ cl_suspend(SCR *sp, int *allowedp) >> * unchanged. In addition, the terminal has already been reset >> * correctly, so leave it alone. >> */ >> - if (clp->killersig) { >> + if (cl_sigterm) { >> F_CLR(clp, CL_SCR_EX_INIT | CL_SCR_VI_INIT); >> return (0); >> } >> Index: cl/cl_main.c >> =================================================================== >> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v >> retrieving revision 1.34 >> diff -u -p -r1.34 cl_main.c >> --- cl/cl_main.c 1 Sep 2021 14:28:15 -0000 1.34 >> +++ cl/cl_main.c 1 Sep 2021 17:15:34 -0000 >> @@ -33,7 +33,6 @@ >> >> GS *__global_list; /* GLOBAL: List of screens. */ >> >> -volatile sig_atomic_t cl_sighup; >> volatile sig_atomic_t cl_sigint; >> volatile sig_atomic_t cl_sigterm; >> volatile sig_atomic_t cl_sigwinch; >> @@ -120,9 +119,9 @@ main(int argc, char *argv[]) >> } >> >> /* If a killer signal arrived, pretend we just got it. */ >> - if (clp->killersig) { >> - (void)signal(clp->killersig, SIG_DFL); >> - (void)kill(getpid(), clp->killersig); >> + if (cl_sigterm) { >> + (void)signal(cl_sigterm, SIG_DFL); >> + (void)kill(getpid(), cl_sigterm); >> /* NOTREACHED */ >> } >> >> @@ -215,17 +214,6 @@ term_init(char *ttype) >> } >> } >> >> -#define GLOBAL_CLP \ >> - CL_PRIVATE *clp = GCLP(__global_list); >> -static void >> -h_hup(int signo) >> -{ >> - GLOBAL_CLP; >> - >> - cl_sighup = 1; >> - clp->killersig = SIGHUP; >> -} >> - >> static void >> h_int(int signo) >> { >> @@ -235,10 +223,7 @@ h_int(int signo) >> static void >> h_term(int signo) >> { >> - GLOBAL_CLP; >> - >> - cl_sigterm = 1; >> - clp->killersig = SIGTERM; >> + cl_sigterm = signo; >> } >> >> static void >> @@ -246,7 +231,6 @@ h_winch(int signo) >> { >> cl_sigwinch = 1; >> } >> -#undef GLOBAL_CLP >> >> /* >> * sig_init -- >> @@ -261,20 +245,19 @@ sig_init(GS *gp, SCR *sp) >> >> clp = GCLP(gp); >> >> - cl_sighup = 0; >> cl_sigint = 0; >> cl_sigterm = 0; >> cl_sigwinch = 0; >> >> if (sp == NULL) { >> - if (setsig(SIGHUP, &clp->oact[INDX_HUP], h_hup) || >> + if (setsig(SIGHUP, &clp->oact[INDX_HUP], h_term) || >> setsig(SIGINT, &clp->oact[INDX_INT], h_int) || >> setsig(SIGTERM, &clp->oact[INDX_TERM], h_term) || >> setsig(SIGWINCH, &clp->oact[INDX_WINCH], h_winch) >> ) >> err(1, NULL); >> } else >> - if (setsig(SIGHUP, NULL, h_hup) || >> + if (setsig(SIGHUP, NULL, h_term) || >> setsig(SIGINT, NULL, h_int) || >> setsig(SIGTERM, NULL, h_term) || >> setsig(SIGWINCH, NULL, h_winch) >> Index: cl/cl_read.c >> =================================================================== >> RCS file: /cvs/src/usr.bin/vi/cl/cl_read.c,v >> retrieving revision 1.22 >> diff -u -p -r1.22 cl_read.c >> --- cl/cl_read.c 1 Sep 2021 14:28:15 -0000 1.22 >> +++ cl/cl_read.c 1 Sep 2021 17:15:34 -0000 >> @@ -62,13 +62,15 @@ retest: if (LF_ISSET(EC_INTERRUPT) || cl >> evp->e_event = E_TIMEOUT; >> return (0); >> } >> - if (cl_sighup) { >> + switch (cl_sigterm) { >> + case SIGHUP: >> evp->e_event = E_SIGHUP; >> return (0); >> - } >> - if (cl_sigterm) { >> + case SIGTERM: >> evp->e_event = E_SIGTERM; >> return (0); >> + default: >> + break; >> } >> if (cl_sigwinch) { >> cl_sigwinch = 0;