[FFmpeg-devel,1/2] ffmpeg: use sigaction() instead of signal() on linux

From: Andriy Gelman <andriy.gelman@gmail.com>

As per signal() help (man 2 signal) the semantics of using signal may
vary across platforms. It is suggested to use sigaction() instead.

On my system, the capture signal is reset to the default handler after
the first call thus failing to properly handle multiple SIGINTs.

Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
---
 fftools/ffmpeg.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

On Sat, 28. Nov 14:46, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> As per signal() help (man 2 signal) the semantics of using signal may
> vary across platforms. It is suggested to use sigaction() instead.
> 
> On my system, the capture signal is reset to the default handler after
> the first call thus failing to properly handle multiple SIGINTs.
> 
> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> ---
>  fftools/ffmpeg.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 80f436eab3..01f4ef15d8 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -393,8 +393,30 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
>  }
>  #endif
>  
> +#ifdef __linux__
> +#define SIGNAL(sig, func)               \
> +    do {                                \
> +        action.sa_handler = func;       \
> +        sigaction(sig, &action, NULL);  \
> +    } while (0)
> +#else
> +#define SIGNAL(sig, func) \
> +    signal(sig, func)
> +#endif
> +
>  void term_init(void)
>  {
> +#if defined __linux__
> +    struct sigaction action;
> +    action.sa_handler = sigterm_handler;
> +
> +    /* block other interrupts while processing this one */
> +    sigfillset(&action.sa_mask);
> +
> +    /* restart interruptible functions (i.e. don't fail with EINTR)  */
> +    action.sa_flags = SA_RESTART;
> +#endif
> +
>  #if HAVE_TERMIOS_H
>      if (!run_as_daemon && stdin_interaction) {
>          struct termios tty;
> @@ -413,14 +435,15 @@ void term_init(void)
>  
>              tcsetattr (0, TCSANOW, &tty);
>          }
> -        signal(SIGQUIT, sigterm_handler); /* Quit (POSIX).  */
> +        SIGNAL(SIGQUIT, sigterm_handler); /* Quit (POSIX).  */
>      }
>  #endif
>  
> -    signal(SIGINT , sigterm_handler); /* Interrupt (ANSI).    */
> -    signal(SIGTERM, sigterm_handler); /* Termination (ANSI).  */
> +    SIGNAL(SIGINT, sigterm_handler);
> +    SIGNAL(SIGTERM, sigterm_handler);
> +
>  #ifdef SIGXCPU
> -    signal(SIGXCPU, sigterm_handler);
> +    SIGNAL(SIGXCPU, sigterm_handler);
>  #endif
>  #ifdef SIGPIPE
>      signal(SIGPIPE, SIG_IGN); /* Broken pipe (POSIX). */

ping

--
Andriy

On Sun, 13. Dec 11:41, Andriy Gelman wrote:
> On Sat, 28. Nov 14:46, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > As per signal() help (man 2 signal) the semantics of using signal may
> > vary across platforms. It is suggested to use sigaction() instead.
> > 
> > On my system, the capture signal is reset to the default handler after
> > the first call thus failing to properly handle multiple SIGINTs.
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> > ---
> >  fftools/ffmpeg.c | 31 +++++++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 80f436eab3..01f4ef15d8 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -393,8 +393,30 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
> >  }
> >  #endif
> >  
> > +#ifdef __linux__
> > +#define SIGNAL(sig, func)               \
> > +    do {                                \
> > +        action.sa_handler = func;       \
> > +        sigaction(sig, &action, NULL);  \
> > +    } while (0)
> > +#else
> > +#define SIGNAL(sig, func) \
> > +    signal(sig, func)
> > +#endif
> > +
> >  void term_init(void)
> >  {
> > +#if defined __linux__
> > +    struct sigaction action;
> > +    action.sa_handler = sigterm_handler;
> > +
> > +    /* block other interrupts while processing this one */
> > +    sigfillset(&action.sa_mask);
> > +
> > +    /* restart interruptible functions (i.e. don't fail with EINTR)  */
> > +    action.sa_flags = SA_RESTART;
> > +#endif
> > +
> >  #if HAVE_TERMIOS_H
> >      if (!run_as_daemon && stdin_interaction) {
> >          struct termios tty;
> > @@ -413,14 +435,15 @@ void term_init(void)
> >  
> >              tcsetattr (0, TCSANOW, &tty);
> >          }
> > -        signal(SIGQUIT, sigterm_handler); /* Quit (POSIX).  */
> > +        SIGNAL(SIGQUIT, sigterm_handler); /* Quit (POSIX).  */
> >      }
> >  #endif
> >  
> > -    signal(SIGINT , sigterm_handler); /* Interrupt (ANSI).    */
> > -    signal(SIGTERM, sigterm_handler); /* Termination (ANSI).  */
> > +    SIGNAL(SIGINT, sigterm_handler);
> > +    SIGNAL(SIGTERM, sigterm_handler);
> > +
> >  #ifdef SIGXCPU
> > -    signal(SIGXCPU, sigterm_handler);
> > +    SIGNAL(SIGXCPU, sigterm_handler);
> >  #endif
> >  #ifdef SIGPIPE
> >      signal(SIGPIPE, SIG_IGN); /* Broken pipe (POSIX). */
> 
> ping
> 

ping

On 29/11/20 5:46 am, Andriy Gelman wrote:

>   void term_init(void)
>   {
> +#if defined __linux__
> +    struct sigaction action;

Nit: Should this have a "= {0}"?

My sigaction(2) says:
   On some architectures a union is involved: do not assign to both sa_handler and sa_sigaction.
so it's possible that sa_sigaction is left uninitialised.

If I'm wrong (quite possible, it's 2am), then part 1 lgtm.

> +    action.sa_handler = sigterm_handler;
> +
> +    /* block other interrupts while processing this one */
> +    sigfillset(&action.sa_mask);
> +
> +    /* restart interruptible functions (i.e. don't fail with EINTR)  */
> +    action.sa_flags = SA_RESTART;
> +#endif

On Sun, 10. Jan 16:32, Zane van Iperen wrote:
> On 29/11/20 5:46 am, Andriy Gelman wrote:
> 
> >   void term_init(void)
> >   {
> > +#if defined __linux__
> > +    struct sigaction action;

Hi Zane,

Thanks for reviewing the patch.

> 
> Nit: Should this have a "= {0}"?
> 
> My sigaction(2) says:
>   On some architectures a union is involved: do not assign to both sa_handler and sa_sigaction.
> so it's possible that sa_sigaction is left uninitialised.
> 
> If I'm wrong (quite possible, it's 2am), then part 1 lgtm.
> 

SA_SIGINFO in sa_flags used to decide whether sa_handler or sa_sigaction is
chosen from the union.

But, there is one function pointer sa_restorer in struct sigaction that's
currently not initialized. The docs say this pointer is used internally by
glibc/kernel, and should not be used by applications.  It doesn't say that it
needs to be set to NULL, but I suppose it's a good practise.  I'll add your
suggestion to the patch.

Will apply the patch in a few days unless there are other comments.