[libvirt] [PATCH] use sa_handler instead of sa_sigaction when SA_SIGINFO is not defined

[Eric Blake]

On 04/19/2012 09:10 PM, Wen Congyang wrote:
> POSIX requires that we use sa_sigaction if sa_flags includes SA_SIGINFO,
> and that we use sa_handler otherwise. But we still use sa_sigaction
> when SA_SIGINFO is not defined. Practice says it will work, but theory
> says we can't rely on it to work.

Not quite what I said.  Gnulib guarantees that sa_sigaction will work
instead of sa_handler, _for the platforms where SA_SIGINFO is not
defined_, and provided that your handler does not access the 2nd or 3rd
argument in those situations.  That is, gnulib gives us some guarantees
that POSIX does not, and that allows us to write simpler code that
assumes SA_SIGINFO just works everywhere, as long as the handler is
careful with the last two arguments.  For all platforms where SA_SIGINFO
is defined, using just sa_sigaction is fine.

tools/virsh.c is therefore correct, and only src/rpc/virnetserver.c has
a bug.

> 
> ---
>  src/rpc/virnetserver.c |   25 +++++++++++++++++++++++--
>  tools/virsh.c          |   35 ++++++++++++++++++++++++++++-------
>  2 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 3965fc2..131ecb4 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -270,8 +270,12 @@ error:
>  }
>  
>  
> +#ifdef SA_SIGINFO
>  static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED,
>                                      void *context ATTRIBUTE_UNUSED)
> +#else
> +static void virNetServerFatalSignal(int sig)
> +#endif

Yuck.  I'm happy with a single three-arg definition as long as we follow
the gnulib rules about using sa_sigaction and SA_SIGINFO as a pair,
rather than adding ugly #ifdefs.

>  {
>      struct sigaction sig_action;
>      int origerrno;
> @@ -286,6 +290,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED
>  #ifdef SIGUSR2
>      if (sig != SIGUSR2) {
>  #endif
> +        memset(&sig_action, 0, sizeof(sig_action));
>          sig_action.sa_handler = SIG_DFL;
>          sigaction(sig, &sig_action, NULL);

Good catch - that memset (or an explicit setting of sig_action.sa_flags
= 0) is required; just because Linux behaves sanely for
sa_handler==SIG_DFL regardless of uninitialized sa_flags does not mean
all platforms do likewise.

>          raise(sig);
> @@ -362,7 +367,12 @@ virNetServerPtr virNetServerNew(size_t min_workers,
>       * catch fatal errors to dump a log, also hook to USR2 for dynamic
>       * debugging purposes or testing
>       */
> +#ifdef SA_SIGINFO
>      sig_action.sa_sigaction = virNetServerFatalSignal;
> +    sig_action.sa_flags = SA_SIGINFO;

This one line is important,

> +#else
> +    sig_action.sa_handler = virNetServerFatalSignal;
> +#endif

this #ifdef stuff is just gross.  We should instead copy virsh.c and
#define SA_SIGINFO 0 if it is not already defined.

>      sigaction(SIGFPE, &sig_action, NULL);
>      sigaction(SIGSEGV, &sig_action, NULL);
>      sigaction(SIGILL, &sig_action, NULL);
> @@ -420,17 +430,28 @@ static sig_atomic_t sigErrors = 0;
>  static int sigLastErrno = 0;
>  static int sigWrite = -1;
>  
> +#ifdef SA_SIGINFO
>  static void virNetServerSignalHandler(int sig, siginfo_t * siginfo,
>                                        void* context ATTRIBUTE_UNUSED)
> +#else
> +static void virNetServerSignalHandler(int sig)
> +#endif

Not sure I like the #ifdef here,

>  {
>      int origerrno;
>      int r;
> +    siginfo_t temp_siginfo;
> +
> +#ifdef SA_SIGINFO
> +    memcpy(&temp_siginfo, siginfo, sizeof(temp_siginfo));
> +#else
> +    memset(&temp_siginfo, 0, sizeof(temp_siginfo));
> +#endif

Here, if you start the file with:

#ifndef SA_SIGINFO
# define SA_SIGINFO 0
#endif

then here, you can avoid the in-function #ifdef with:

    if (SA_SIGINFO)
        memcpy(&tmp_siginfo, siginfo, sizeof(temp_siginfo));
    else
        memset(&temp_siginfo, 0, sizeof(temp_siginfo));

>  
>      /* set the sig num in the struct */
> -    siginfo->si_signo = sig;
> +    temp_siginfo.si_signo = sig;
>  
>      origerrno = errno;
> -    r = safewrite(sigWrite, siginfo, sizeof(*siginfo));
> +    r = safewrite(sigWrite, &temp_siginfo, sizeof(temp_siginfo));

This change makes sense (the caller won't get much more than si_siginfo
out of their virNetServerSignalFunc callback, but at least we are no
longer passing random memory to the callback on mingw).

> +++ b/tools/virsh.c
> @@ -579,9 +579,13 @@ out:
>  
>  static volatile sig_atomic_t intCaught = 0;
>  
> +#ifdef SA_SIGINFO
>  static void vshCatchInt(int sig ATTRIBUTE_UNUSED,
>                          siginfo_t *siginfo ATTRIBUTE_UNUSED,
>                          void *context ATTRIBUTE_UNUSED)
> +#else
> +static void vshCatchInt(int sig ATTRIBUTE_UNUSED)
> +#endif

Not needed.

>  {
>      intCaught = 1;
>  }
> @@ -591,23 +595,25 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED,
>   */
>  static int disconnected = 0; /* we may have been disconnected */
>  
> -/* Gnulib doesn't guarantee SA_SIGINFO support.  */
> -#ifndef SA_SIGINFO
> -# define SA_SIGINFO 0
> -#endif

Keep this, and copy it to virnetserver.c (or even float it to a common
location, like internal.h).

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120419/778e17f3/attachment-0001.sig>