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

[Wen Congyang]

At 04/20/2012 11:26 AM, Eric Blake Wrote:
> 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.

Sorry for my misunderstanding. I will update this patch.

Thanks
Wen Congyang

> 
> 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).
>