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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160217/c9888565/attachment.sig>