Hi Tim,
trondd wrote on Wed, Sep 01, 2021 at 08:46:28PM -0400:
> Ingo Schwarze <[email protected]> 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;
