On Thu, 2021-09-02 at 13:25 +0200, Ingo Schwarze wrote: > 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?
Luckily the paint is pretty. :-) > > 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; >