Hi Paul, thanks for the review:
Paul Eggert <[email protected]> writes: > Gisle Vanem wrote: >> No. He added a 'reset_color_context (true);' inside >> 'signal_handler()'. > > Ah, sorry, I was looking at the original edition of the patch. > > I just now looked at <http://bugs.gnu.org/20062#101> and still see > some problems. Mainly, it doesn't handle SIGTSTP properly. With > SIGTSTP an application is supposed to reset the terminal and then > resignal itself with SIGSTOP before really stopping. When restarting > the application then needs to restore the terminal to the current > state, if the current state is not the default. This is nearly > impossible to do correctly with the proposed design. I suggest > looking at how GNU ls does it: the signal handler merely sets a flag, > and the application checks occasionally at safe places whether a > signal has arrived. IIRC we agreed on this design so that we react immediately to signals without blocking (now the most we can block is to write the reset sequence, but we can't cut this), blocking signals was done in an older version of the patch. I've tried to handle SIGTSTP with this addition and everything seems to work as expected, I'll test it more before propose a new version of the patch. Do you see any issue with it? Regards, Giuseppe diff --git a/src/util.c b/src/util.c index 0f8f7a8..7f11318 100644 --- a/src/util.c +++ b/src/util.c @@ -28,6 +28,8 @@ char const pr_program[] = PR_PROGRAM; +static const char *last_context_str; + /* Queue up one-line messages to be printed at the end, when -l is specified. Each message is recorded with a 'struct msg'. */ @@ -145,8 +147,35 @@ print_message_queue (void) } static void +safe_write_from_signal (const char *str, size_t len) +{ + size_t written = 0; + while (written < len) + { + int ret = write (STDOUT_FILENO, str + written, + len - written); + if (ret < 0) + return; + written += ret; + } +} + +static void signal_handler (int sig) { + if (sig == SIGCONT) + { + if (last_context_str) + safe_write_from_signal (last_context_str, strlen (last_context_str)); + return; + } + if (sig == SIGTSTP) + { + reset_color_context (true); + raise (SIGSTOP); + return; + } + reset_color_context (true); /* Restore the default handler, and report the signal again. */ @@ -163,6 +192,7 @@ install_signal_handlers (void) #endif int const sig[] = { + SIGCONT, SIGTSTP, SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM, #ifdef SIGPOLL @@ -796,28 +826,40 @@ void set_header_color_context (void) { if (colors_enabled) - fprintf (outfile, "\x1B[1;39m"); + { + last_context_str = "\x1B[1;39m"; + fprintf (outfile, "%s", last_context_str); + } } void set_line_numbers_color_context (void) { if (colors_enabled) - fprintf (outfile, "\x1B[36m"); + { + last_context_str = "\x1B[36m"; + fprintf (outfile, "%s", last_context_str); + } } void set_add_color_context (void) { if (colors_enabled) - fprintf (outfile, "\x1B[32m"); + { + last_context_str = "\x1B[32m"; + fprintf (outfile, "%s", last_context_str); + } } void set_delete_color_context (void) { if (colors_enabled) - fprintf (outfile, "\x1B[31m"); + { + last_context_str = "\x1B[31m"; + fprintf (outfile, "%s", last_context_str); + } } void @@ -827,19 +869,12 @@ reset_color_context (bool from_signal) if (! colors_enabled) return; - if (! from_signal) - fputs (reset_sequence, outfile); + if (from_signal) + safe_write_from_signal (reset_sequence, sizeof reset_sequence - 1); else { - size_t written = 0; - while (written < sizeof reset_sequence - 1) - { - int ret = write (STDOUT_FILENO, reset_sequence + written, - sizeof reset_sequence - 1 - written); - if (ret < 0) - return; - written += ret; - } + fputs (reset_sequence, outfile); + last_context_str = NULL; } }
