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

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

[Gerd Hoffmann]

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;
+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;
-- 
1.8.3.1

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

[Laszlo Ersek]

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>

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

[Gerd Hoffmann]

  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

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

[Paolo Bonzini]

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

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

[Laszlo Ersek]

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