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).
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list