Ingo Schwarze <schwa...@usta.de> wrote:
> Hi,
>
> 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.
>
> OK?
> Ingo
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.
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.
Tim. (not @ :P )
>
>
> 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;
