comments below On 11/12/13 09:43, Gerd Hoffmann wrote: > Don't run code in the signal handler, only set a flag. > Use sigaction(2) to avoid non-portable signal(2) semantics. > Make #ifdefs less messy. > > Signed-off-by: Gerd Hoffmann <kra...@redhat.com> > --- > ui/curses.c | 44 ++++++++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/ui/curses.c b/ui/curses.c > index 289a955..c33acff 100644 > --- a/ui/curses.c > +++ b/ui/curses.c > @@ -106,9 +106,9 @@ static void curses_resize(DisplayChangeListener *dcl, > curses_calc_pad(); > } > > -#ifndef _WIN32 > -#if defined(SIGWINCH) && defined(KEY_RESIZE) > -static void curses_winch_handler(int signum) > +#if !defined(_WIN32) && defined(SIGWINCH) && defined(KEY_RESIZE) > +static bool got_sigwinch;
The type "volatile sig_atomic_t" would be more pedantic <http://pubs.opengroup.org/onlinepubs/9699919799//functions/V2_chap02.html#tag_15_04_03_03>. > +static void curses_winch_check(void) > { > struct winsize { > unsigned short ws_row; > @@ -117,18 +117,34 @@ static void curses_winch_handler(int signum) > unsigned short ws_ypixel; /* unused */ > } ws; > > - /* terminal size changed */ > - if (ioctl(1, TIOCGWINSZ, &ws) == -1) > + if (!got_sigwinch) { > + return; > + } > + got_sigwinch = false; > + > + if (ioctl(1, TIOCGWINSZ, &ws) == -1) { > return; > + } > > resize_term(ws.ws_row, ws.ws_col); > - curses_calc_pad(); Are you removing this call because we're setting "invalidate" below, and the (new) caller of this function, curses_refresh(), calls curses_calc_pad() on nonzero "invalidate" anyway? > invalidate = 1; > +} Also, I grepped the source for SIGWINCH, and I think it is never masked with pthread_sigmask(), or -- while the process is single-threaded initially -- with sigprocmask(). Hence this signal can be delivered at any time and interrupt interruptible functions (which is good justification for the patch.) My point though is that after this patch a narrow window seems to exist where you can lose a signal, namely between checking "got_sigwinch" and resetting it. (SIGWINCH is not a standard signal but I do think it it's not a realtime one, hence it doesn't queue up; it can only be pending or not.) For ultimate pedantry we could maybe write bool local; /* block the signal with pthread_sigmask() * for atomic retrieval and reset */ local = got_sigwinch; got_sigwinch = false; /* unblock the signal */ if (!local) { return; } but it's likely overkill. > > - /* some systems require this */ > - signal(SIGWINCH, curses_winch_handler); > +static void curses_winch_handler(int signum) > +{ > + got_sigwinch = true; > } > -#endif > + > +static void curses_winch_init(void) > +{ > + struct sigaction old, winch = { > + .sa_handler = curses_winch_handler, > + }; > + sigaction(SIGWINCH, &winch, &old); > +} You don't need "old" here (you don't use it to restore the handler), just pass NULL. > +#else > +static void curses_winch_check(void) {} > +static void curses_winch_init(void) {} > #endif > > static void curses_cursor_position(DisplayChangeListener *dcl, > @@ -163,6 +179,8 @@ static void curses_refresh(DisplayChangeListener *dcl) > { > int chr, nextchr, keysym, keycode, keycode_alt; > > + curses_winch_check(); > + > if (invalidate) { > clear(); > refresh(); > @@ -349,13 +367,7 @@ void curses_display_init(DisplayState *ds, int > full_screen) > curses_keyboard_setup(); > atexit(curses_atexit); > > -#ifndef _WIN32 > -#if defined(SIGWINCH) && defined(KEY_RESIZE) > - /* some curses implementations provide a handler, but we > - * want to be sure this is handled regardless of the library */ > - signal(SIGWINCH, curses_winch_handler); > -#endif > -#endif > + curses_winch_init(); > > dcl = (DisplayChangeListener *) g_malloc0(sizeof(DisplayChangeListener)); > dcl->ops = &dcl_ops; > If you think a v2 is not warranted for (and because the patch is strictly an improvement): Reviewed-by: Laszlo Ersek <ler...@redhat.com>
