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

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