From: Florian Obser Subject: Re: watch(1) - periodically execute a command and display its output To: Ingo Schwarze Cc: Theo de Raadt , Job Snijders , tech@openbsd.org Date: Wed, 21 May 2025 06:38:37 +0200 On 2025-05-21 04:28 +02, Ingo Schwarze 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 diff --git watch.c watch.c index 5b709f98d95..a2c533e4f03 100644 --- watch.c +++ watch.c @@ -19,11 +19,13 @@ */ #include +#include #include #include #include #include +#include #include #include #include @@ -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.