On 2025-05-21 04:28 +02, Ingo Schwarze <schwarze@usta.de> wrote:
> Hi Theo,
>
> Theo de Raadt wrote on Mon, May 19, 2025 at 06:50:03PM -0600:
>
>> Over my dead body!
>
> Just trying to save your life, 'cause it would be such a pity...  ;-)
>
> I think there is nothing wrong with checking for signals at multiple
> points in the command_loop(), for example before every operation
> that might potentially take a significant time.
>
> Also, the logical place to put cleanup code is at the end of main().
>
> To be committed after florian@'s cmdstr patch goes in, of course.
>
> OK?
>   Ingo
>
>
> --- watch.c	Wed May 21 04:12:27 2025
> +++ watch.c	Wed May 21 04:09:20 2025
> @@ -50,7 +50,8 @@ typedef enum {
>  	RSLT_UPDATE,
>  	RSLT_REDRAW,
>  	RSLT_NOTOUCH,
> -	RSLT_ERROR
> +	RSLT_ERROR,
> +	RSLT_QUIT
>  }    kbd_result_t;
>  
>  /*
> @@ -64,6 +65,8 @@ struct {
>  	.d = DEFAULT_INTERVAL
>  };
>  
> +volatile sig_atomic_t got_signal = 0;
> +
>  highlight_mode_t highlight_mode = HIGHLIGHT_NONE;
>  highlight_mode_t last_highlight_mode = HIGHLIGHT_CHAR;
>  
> @@ -93,7 +96,6 @@ kbd_result_t kbd_command(int);
>  void show_help(void);
>  void untabify(wchar_t *, int);
>  void on_signal(int);
> -void quit(void);
>  void usage(void);
>  
>  int
> @@ -206,8 +208,12 @@ main(int argc, char *argv[])
>  
>  	command_loop();
>  
> -	/* NOTREACHED */
> -	abort();
> +	erase();
> +	refresh();
> +	endwin();
> +	free(cmdv);
> +
> +	return got_signal ? 128 + got_signal : 0;
>  }
>  
>  void
> @@ -231,12 +237,21 @@ command_loop(void)
>  			prev = &buf0;
>  		}
>  
> +		if (got_signal)
> +			break;
> +
>  		read_result(cur);
>  
>  redraw:
> +		if (got_signal)
> +			break;
> +
>  		display(cur, prev, highlight_mode);
>  
>  input:
> +		if (got_signal)
> +			break;
> +
>  		to = opt_interval.tv;
>  		FD_ZERO(&readfds);
>  		FD_SET(fileno(stdin), &readfds);
> @@ -269,6 +284,8 @@ input:
>  			case RSLT_ERROR:	/* error */
>  				fprintf(stderr, "\007");
>  				goto input;
> +			case RSLT_QUIT:
> +				return;
>  			}
>  		}
>  	}
> @@ -595,8 +612,7 @@ kbd_command(int ch)
>  		break;
>  
>  	case 'q':
> -		quit();
> -		break;
> +		return (RSLT_QUIT);
>  
>  	default:
>  		return (RSLT_ERROR);
> @@ -682,17 +698,7 @@ untabify(wchar_t *buf, int maxlen)
>  void
>  on_signal(int signum)
>  {
> -	quit();
> -}
> -
> -void
> -quit(void)
> -{
> -	erase();
> -	refresh();
> -	endwin();
> -	free(cmdv);
> -	exit(0);
> +	got_signal = signum;
>  }
>  
>  void
>

I was already working on moving watch to libevent.
Your diff is shorter, mine gets rid of select, but I suppose someone
(not me, I have no experience) could also replace select with poll.

Since my diff is done:

diff --git Makefile Makefile
index 55e95cf5e1d..fd882879d2a 100644
--- Makefile
+++ Makefile
@@ -5,7 +5,7 @@ PROG=	watch
 # XXX: why is this needed?
 CFLAGS+= -D_XOPEN_SOURCE_EXTENDED -Wall
 
-LDADD+= -lcurses
-DPADD+= ${LIBCURSES}
+LDADD+= -lcurses -levent
+DPADD+= ${LIBCURSES} ${LIBEVENT}
 
 .include <bsd.prog.mk>
diff --git watch.c watch.c
index 5b709f98d95..a2c533e4f03 100644
--- watch.c
+++ watch.c
@@ -19,11 +19,13 @@
  */
 
 #include <sys/types.h>
+#include <sys/ioctl.h>
 #include <sys/wait.h>
 
 #include <curses.h>
 #include <err.h>
 #include <errno.h>
+#include <event.h>
 #include <locale.h>
 #include <paths.h>
 #include <signal.h>
@@ -80,21 +82,26 @@ static char	 *cmdstr;
 static size_t	  cmdlen;
 static char	**cmdv;
 
-typedef wchar_t BUFFER[MAXLINE][MAXCOLUMN + 1];
+typedef wchar_t	  BUFFER[MAXLINE][MAXCOLUMN + 1];
+BUFFER		  buf0, buf1;
+BUFFER		 *cur_buf, *prev_buf;
+struct event	  ev_timer;
 
 #define MAXIMUM(a, b)	(((a) > (b)) ? (a) : (b))
 #define MINIMUM(a, b)	(((a) < (b)) ? (a) : (b))
 
 #define ctrl(c)		((c) & 037)
-void command_loop(void);
 int display(BUFFER *, BUFFER *, highlight_mode_t);
 void read_result(BUFFER *);
 kbd_result_t kbd_command(int);
 void show_help(void);
 void untabify(wchar_t *, int);
-void on_signal(int);
+void on_signal(int, short, void *);
+void timer(int, short, void *);
+void input(int, short, void *);
 void quit(void);
 void usage(void);
+void swap_buffers(void);
 
 int
 set_interval(const char *str)
@@ -115,6 +122,7 @@ set_interval(const char *str)
 int
 main(int argc, char *argv[])
 {
+	struct event ev_sigint, ev_sighup, ev_sigterm, ev_sigwinch, ev_stdin;
 	size_t len, rem;
 	int i, ch;
 	char *p;
@@ -197,83 +205,35 @@ main(int argc, char *argv[])
 	crmode();
 	keypad(stdscr, TRUE);
 
+	event_init();
+
 	/*
 	 * Initialize signal
 	 */
-	(void) signal(SIGINT, on_signal);
-	(void) signal(SIGTERM, on_signal);
-	(void) signal(SIGHUP, on_signal);
-
-	command_loop();
+	signal_set(&ev_sigint, SIGINT, on_signal, NULL);
+	signal_set(&ev_sighup, SIGHUP, on_signal, NULL);
+	signal_set(&ev_sigterm, SIGTERM, on_signal, NULL);
+	signal_set(&ev_sigwinch, SIGWINCH, on_signal, NULL);
+	signal_add(&ev_sigint, NULL);
+	signal_add(&ev_sighup, NULL);
+	signal_add(&ev_sigterm, NULL);
+	signal_add(&ev_sigwinch, NULL);
+
+	event_set(&ev_stdin, STDIN_FILENO, EV_READ | EV_PERSIST, input, NULL);
+	event_add(&ev_stdin, NULL);
+	evtimer_set(&ev_timer, timer, NULL);
+	evtimer_add(&ev_timer, &opt_interval.tv);
+
+	cur_buf = &buf0; prev_buf = &buf1;
+	read_result(cur_buf);
+	display(cur_buf, prev_buf, highlight_mode);
+
+	event_dispatch();
 
 	/* NOTREACHED */
 	abort();
 }
 
-void
-command_loop(void)
-{
-	int		 i, nfds;
-	BUFFER		 buf0, buf1;
-	fd_set		 readfds;
-	struct timeval	 to;
-
-	for (i = 0; ; i++) {
-		BUFFER *cur, *prev;
-
-		if (i == 0) {
-			cur = prev = &buf0;
-		} else if (i % 2 == 0) {
-			cur = &buf0;
-			prev = &buf1;
-		} else {
-			cur = &buf1;
-			prev = &buf0;
-		}
-
-		read_result(cur);
-
-redraw:
-		display(cur, prev, highlight_mode);
-
-input:
-		to = opt_interval.tv;
-		FD_ZERO(&readfds);
-		FD_SET(fileno(stdin), &readfds);
-		nfds = select(1, &readfds, NULL, NULL,
-		    (pause_status)? NULL : &to);
-		if (nfds < 0)
-			switch (errno) {
-			case EINTR:
-				/*
-				 * ncurses has changed the window size with
-				 * SIGWINCH.  call doupdate() to use the
-				 * updated window size.
-				 */
-				doupdate();
-				goto redraw;
-			default:
-				perror("select");
-			}
-		else if (nfds > 0) {
-			int ch = getch();
-			kbd_result_t result = kbd_command(ch);
-
-			switch (result) {
-			case RSLT_UPDATE:	/* update buffer */
-				break;
-			case RSLT_REDRAW:	/* scroll with current buffer */
-				goto redraw;
-			case RSLT_NOTOUCH:	/* silently loop again */
-				goto input;
-			case RSLT_ERROR:	/* error */
-				fprintf(stderr, "\007");
-				goto input;
-			}
-		}
-	}
-}
-
 int
 display(BUFFER * cur, BUFFER * prev, highlight_mode_t hm)
 {
@@ -551,10 +511,13 @@ kbd_command(int ch)
 		break;
 
 	case 'p':
-		if ((pause_status = !pause_status) != 0)
+		if ((pause_status = !pause_status) != 0) {
+			evtimer_del(&ev_timer);
 			return (RSLT_REDRAW);
-		else
+		} else {
+			evtimer_add(&ev_timer, &opt_interval.tv);
 			return (RSLT_UPDATE);
+		}
 
 	case 's':
 		move(1, 0);
@@ -575,6 +538,7 @@ kbd_command(int ch)
 			refresh();
 			return (RSLT_ERROR);
 		}
+		evtimer_add(&ev_timer, &opt_interval.tv);
 
 		return (RSLT_REDRAW);
 
@@ -679,10 +643,65 @@ untabify(wchar_t *buf, int maxlen)
 	*p = L'\0';
 }
 
+void swap_buffers(void) {
+	BUFFER *t;
+	t = prev_buf;
+	prev_buf = cur_buf;
+	cur_buf = t;
+}
+
 void
-on_signal(int signum)
+on_signal(int sig, short event, void *arg)
 {
-	quit();
+	struct winsize ws;
+
+	switch(sig) {
+	case SIGWINCH:
+		if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) != -1)
+			resizeterm(ws.ws_row, ws.ws_col);
+		doupdate();
+		display(cur_buf, prev_buf, highlight_mode);
+		break;
+	default:
+		quit();
+	}
+}
+
+void
+timer(int sig, short event, void *arg)
+{
+	swap_buffers();
+	read_result(cur_buf);
+	display(cur_buf, prev_buf, highlight_mode);
+	if (!pause_status)
+		evtimer_add(&ev_timer, &opt_interval.tv);
+}
+
+void
+input(int sig, short event, void *arg)
+{
+	int ch;
+
+	ch = getch();
+
+	kbd_result_t result = kbd_command(ch);
+	switch (result) {
+	case RSLT_UPDATE:	/* update buffer */
+		swap_buffers();
+		read_result(cur_buf);
+		display(cur_buf, prev_buf, highlight_mode);
+		if (!pause_status)
+			evtimer_add(&ev_timer, &opt_interval.tv);
+		break;
+	case RSLT_REDRAW:	/* scroll with current buffer */
+		display(cur_buf, prev_buf, highlight_mode);
+		break;
+	case RSLT_NOTOUCH:	/* silently loop again */
+		break;
+	case RSLT_ERROR:	/* error */
+		fprintf(stderr, "\007");
+		break;
+	}
 }
 
 void


-- 
In my defence, I have been left unsupervised.