Message ID | 1384245814-15010-1-git-send-email-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
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 <kraxel@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 <lersek@redhat.com>
Hi, > > +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>. Will do for v2. Hmm, checkpatch barfs on "volatile", with a reference which looks linux kernel related ... > > > > 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? Exactly. > 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. Doesn't matter. The signal just says "terminal size has changed", typically as result of a xterm window resize. Even if we get that twice we have to handle it only once, we just have to make sure this happens after the second signal came in. Which is the case, as we reset got_sigwinch before going to handle it (query new size, tell curses). cheers, Gerd
Il 12/11/2013 12:53, Laszlo Ersek ha scritto: > 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. You don't really lose it, it is delayed to the next refresh. > (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.) Indeed. > 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. Or just use an atomic function: if (!atomic_xchg(&got_sigwinch, false)) { return; } Paolo
On 11/12/13 13:16, Gerd Hoffmann wrote: >> 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. > > Doesn't matter. The signal just says "terminal size has changed", > typically as result of a xterm window resize. Even if we get that twice > we have to handle it only once, we just have to make sure this happens > after the second signal came in. Which is the case, as we reset > got_sigwinch before going to handle it (query new size, tell curses). Ah, correct. We do lose the second signal (the one which was delivered between we retrieve the flag set by the first instance, and resetting the flag), but at that point we're going to handle *one* resize event anyway. Supposing that the second signal was generated *after* the terminal size had changed for the second time, we'll see a recent enough terminal size when we query it. So, we lose the second signal and the *first* terminal size. Good. Thanks! Laszlo
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; +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(); invalidate = 1; +} - /* 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); +} +#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;
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 <kraxel@redhat.com> --- ui/curses.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)