8:17 a.m.
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#...
Thanks,
--
Pino Toscano