Libguestfs - Feb 2016 - [PATCH] fish: reset the console on ^Z RHBZ#1213844

Patch registers SIGTSTP hook where it sends reset terminal color control
sequence.

Maros Zatko (1):
  fish: reset the console on ^Z RHBZ#1213844

 fish/fish.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

-- 
2.5.0

Patch registers SIGTSTP hook where it sends reset terminal color
control sequence.
---
 fish/fish.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/fish/fish.c b/fish/fish.c
index d26f8b3..b579898 100644
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -73,6 +73,11 @@ static void cleanup_readline (void);
 static char *decode_ps1 (const char *);
 static void add_history_line (const char *);
 #endif
+static void set_stophandler (void);
+static void restore_stophandler (void);
+static void user_control_z (int sig);
+
+static void (*otstpfn) = SIG_DFL;
 
 static int override_progress_bars = -1;
 static struct progress_bar *bar = NULL;
@@ -159,6 +164,64 @@ usage (int status)
   exit (status);
 }
 
+/*
+ * Set the TSTP handler.
+ */
+static void
+set_stophandler (void)
+{
+  otstpfn = signal (SIGTSTP, user_control_z);
+}
+
+/*
+ * Restore the TSTP handler.
+ */
+static void
+restore_stophandler (void)
+{
+  signal (SIGTSTP, otstpfn);
+}
+
+static void
+user_control_z (int sig)
+{
+  sigset_t oset, set;
+
+#ifdef HAVE_LIBREADLINE
+  /* Cleanup readline, unhook from terminal */
+  rl_free_line_state ();
+  rl_cleanup_after_signal ();
+#endif
+
+  /* Reset terminal color */
+#define RESETCOLOR "\033[0m"
+  printf (RESETCOLOR);
+  fflush (stdout);
+
+  /* Unblock SIGTSTP. */
+  sigemptyset (&set);
+  sigaddset (&set, SIGTSTP);
+  sigprocmask (SIG_UNBLOCK, &set, NULL);
+
+  restore_stophandler ();
+
+  /* Stop ourselves. */
+  kill (0, SIGTSTP);
+
+  /* Now we're stopped ... */
+
+  /* Reset the curses SIGTSTP signal handler. */
+  set_stophandler ();
+
+#ifdef HAVE_LIBREADLINE
+  /* Restore readline state */
+  rl_reset_after_signal ();
+#endif
+
+  /* Reset the signals. */
+  sigprocmask (SIG_SETMASK, &oset, NULL);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -439,6 +502,15 @@ main (int argc, char *argv[])
       exit (EXIT_FAILURE);
   }
 
+  /* Register ^Z handler. We need it to reset terminal colors
+   */
+  if (is_interactive) {
+    memset (&sa, 0, sizeof sa);
+    sa.sa_handler = user_control_z;
+    sa.sa_flags = SA_RESTART;
+    sigaction (SIGTSTP, &sa, NULL);
+  }
+
   /* Old-style -i syntax?  Since -a/-d/-N and -i was disallowed
    * previously, if we have -i without any drives but with something
    * on the command line, it must be old-style syntax.
-- 
2.5.0

Hi,

On Tuesday 16 February 2016 18:06:44 Maros Zatko wrote:
> Patch registers SIGTSTP hook where it sends reset terminal color
> control sequence.
> ---
IMHO the commit message and the patch (in form of comments) could get
more explanation of what is being done, and why each step is done.
Signal handling is a tricky part of C programming, so everything needs
to be properly documented in order to not break it in the future.
>  fish/fish.c | 72
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/fish/fish.c b/fish/fish.c
> index d26f8b3..b579898 100644
> --- a/fish/fish.c
> +++ b/fish/fish.c
> @@ -73,6 +73,11 @@ static void cleanup_readline (void);
>  static char *decode_ps1 (const char *);
>  static void add_history_line (const char *);
>  #endif
> +static void set_stophandler (void);
> +static void restore_stophandler (void);
> +static void user_control_z (int sig);
> +
> +static void (*otstpfn) = SIG_DFL;
>  
>  static int override_progress_bars = -1;
>  static struct progress_bar *bar = NULL;
> @@ -159,6 +164,64 @@ usage (int status)
>    exit (status);
>  }
>  
> +/*
> + * Set the TSTP handler.
> + */
> +static void
> +set_stophandler (void)
> +{
> +  otstpfn = signal (SIGTSTP, user_control_z);
> +}
> +
> +/*
> + * Restore the TSTP handler.
> + */
> +static void
> +restore_stophandler (void)
> +{
> +  signal (SIGTSTP, otstpfn);
> +}
sigaction() is used to install the signal handler for SIGTSTP, but
signal() is used in these functions (called from the signal handler):
this is a bad, you should not mix sigaction() and signal() usages;
see the sigaction() documentation [1].
> +static void
> +user_control_z (int sig)
> +{
> +  sigset_t oset, set;
> +
> +#ifdef HAVE_LIBREADLINE
> +  /* Cleanup readline, unhook from terminal */
> +  rl_free_line_state ();
> +  rl_cleanup_after_signal ();
> +#endif
> +
> +  /* Reset terminal color */
> +#define RESETCOLOR "\033[0m"
> +  printf (RESETCOLOR);
> +  fflush (stdout);
Neither printf() nor fflush() are async-signal-safe functions, and
thus you cannot use them in a signal handler; see [2] for the list of
supported functions.  You can use write() and fsync() though.
> +  /* Unblock SIGTSTP. */
> +  sigemptyset (&set);
> +  sigaddset (&set, SIGTSTP);
> +  sigprocmask (SIG_UNBLOCK, &set, NULL);
Sounds like this is what the SA_NODEFER flag for sigaction() does.
> +
> +  restore_stophandler ();
> +
> +  /* Stop ourselves. */
> +  kill (0, SIGTSTP);
> +
> +  /* Now we're stopped ... */
> +
> +  /* Reset the curses SIGTSTP signal handler. */
> +  set_stophandler ();
IMHO a better way would be to save the previous SIGTSTP signal handler
(which sigaction() gives you), and call it manually
> +
> +#ifdef HAVE_LIBREADLINE
> +  /* Restore readline state */
> +  rl_reset_after_signal ();
> +#endif
> +
> +  /* Reset the signals. */
> +  sigprocmask (SIG_SETMASK, &oset, NULL);
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -439,6 +502,15 @@ main (int argc, char *argv[])
>        exit (EXIT_FAILURE);
>    }
>  
> +  /* Register ^Z handler. We need it to reset terminal colors
> +   */
> +  if (is_interactive) {
> +    memset (&sa, 0, sizeof sa);
> +    sa.sa_handler = user_control_z;
> +    sa.sa_flags = SA_RESTART;
> +    sigaction (SIGTSTP, &sa, NULL);
> +  }
> +
>    /* Old-style -i syntax?  Since -a/-d/-N and -i was disallowed
>     * previously, if we have -i without any drives but with something
>     * on the command line, it must be old-style syntax.
> 
[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html
[2]
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04

Thanks,
-- 
Pino Toscano