Re: [Qemu-devel] [PATCH] curses: fixup SIGWINCH handler mess

[Laszlo Ersek <lersek@redhat.com>]

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>