[18/21] move blocking of signals to qemu_signalfd_init

This makes it easier to add a Win32 stub.  The patch does that, too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
 1 files changed, 36 insertions(+), 51 deletions(-)

On 2011-02-21 09:43, Paolo Bonzini wrote:
> This makes it easier to add a Win32 stub.  The patch does that, too.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
>  1 files changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 1b6893d..5bb95d8 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -346,11 +346,37 @@ static void sigfd_handler(void *opaque)
>      }
>  }
>  
> -static int qemu_signalfd_init(sigset_t mask)
> +static int qemu_signalfd_init(void)
>  {
>      int sigfd;
> +    sigset_t set;
>  
> -    sigfd = qemu_signalfd(&mask);
> +#ifdef CONFIG_IOTHREAD
> +    /* SIGUSR2 used by posix-aio-compat.c */
> +    sigemptyset(&set);
> +    sigaddset(&set, SIGUSR2);
> +    pthread_sigmask(SIG_UNBLOCK, &set, NULL);

These lines are actually not related to signalfd setup. So you may want
to rename this function or move everything that does not block signals
and then pass the very same set to qemu_signalfd elsewhere.

> +
> +    sigemptyset(&set);
> +    sigaddset(&set, SIGIO);
> +    sigaddset(&set, SIGALRM);
> +    sigaddset(&set, SIG_IPI);
> +    sigaddset(&set, SIGBUS);
> +    pthread_sigmask(SIG_BLOCK, &set, NULL);
> +#else
> +    sigemptyset(&set);
> +    sigaddset(&set, SIGBUS);
> +    if (kvm_enabled()) {
> +        /*
> +         * We need to process timer signals synchronously to avoid a race
> +         * between exit_request check and KVM vcpu entry.
> +         */
> +        sigaddset(&set, SIGIO);
> +        sigaddset(&set, SIGALRM);
> +    }
> +#endif
> +
> +    sigfd = qemu_signalfd(&set);
>      if (sigfd == -1) {
>          fprintf(stderr, "failed to create signalfd\n");
>          return -errno;
> @@ -438,6 +464,12 @@ static void qemu_event_increment(void)
>  static void qemu_kvm_eat_signals(CPUState *env)
>  {
>  }
> +
> +static int qemu_signalfd_init(void)
> +{
> +    return 0;
> +}
> +
>  #endif /* _WIN32 */
>  
>  #ifndef CONFIG_IOTHREAD
> @@ -471,39 +503,14 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>  #endif
>  }
>  
> -#ifndef _WIN32
> -static sigset_t block_synchronous_signals(void)
> -{
> -    sigset_t set;
> -
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGBUS);
> -    if (kvm_enabled()) {
> -        /*
> -         * We need to process timer signals synchronously to avoid a race
> -         * between exit_request check and KVM vcpu entry.
> -         */
> -        sigaddset(&set, SIGIO);
> -        sigaddset(&set, SIGALRM);
> -    }
> -
> -    return set;
> -}
> -#endif
> -
>  int qemu_init_main_loop(void)
>  {
> -#ifndef _WIN32
> -    sigset_t blocked_signals;
>      int ret;
>  
> -    blocked_signals = block_synchronous_signals();
> -
> -    ret = qemu_signalfd_init(blocked_signals);
> +    ret = qemu_signalfd_init();
>      if (ret) {
>          return ret;
>      }
> -#endif
>  
>      qemu_init_sigbus();
>  
> @@ -651,35 +658,13 @@ static void qemu_tcg_init_cpu_signals(void)
>      pthread_sigmask(SIG_UNBLOCK, &set, NULL);
>  }
>  
> -static sigset_t block_io_signals(void)
> -{
> -    sigset_t set;
> -
> -    /* SIGUSR2 used by posix-aio-compat.c */
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGUSR2);
> -    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
> -
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGIO);
> -    sigaddset(&set, SIGALRM);
> -    sigaddset(&set, SIG_IPI);
> -    sigaddset(&set, SIGBUS);
> -    pthread_sigmask(SIG_BLOCK, &set, NULL);
> -
> -    return set;
> -}
> -
>  int qemu_init_main_loop(void)
>  {
>      int ret;
> -    sigset_t blocked_signals;
>  
>      qemu_init_sigbus();
>  
> -    blocked_signals = block_io_signals();
> -
> -    ret = qemu_signalfd_init(blocked_signals);
> +    ret = qemu_signalfd_init();
>      if (ret) {
>          return ret;
>      }

For consistency reasons, you should also refactor the !IOTHREAD case.

Jan

On 02/21/2011 11:12 AM, Jan Kiszka wrote:
> On 2011-02-21 09:43, Paolo Bonzini wrote:
>> This makes it easier to add a Win32 stub.  The patch does that, too.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
>>   1 files changed, 36 insertions(+), 51 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 1b6893d..5bb95d8 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -346,11 +346,37 @@ static void sigfd_handler(void *opaque)
>>       }
>>   }
>>
>> -static int qemu_signalfd_init(sigset_t mask)
>> +static int qemu_signalfd_init(void)
>>   {
>>       int sigfd;
>> +    sigset_t set;
>>
>> -    sigfd = qemu_signalfd(&mask);
>> +#ifdef CONFIG_IOTHREAD
>> +    /* SIGUSR2 used by posix-aio-compat.c */
>> +    sigemptyset(&set);
>> +    sigaddset(&set, SIGUSR2);
>> +    pthread_sigmask(SIG_UNBLOCK,&set, NULL);
>
> These lines are actually not related to signalfd setup. So you may want
> to rename this function or move everything that does not block signals
> and then pass the very same set to qemu_signalfd elsewhere.

Ok, I'll just rename qemu_signalfd_init to qemu_init_iothread_signals.

> For consistency reasons, you should also refactor the !IOTHREAD case.

I think I did?

non-iothread:

>>   int qemu_init_main_loop(void)
>>   {
>> -#ifndef _WIN32
>> -    sigset_t blocked_signals;
>>       int ret;
>>
>> -    blocked_signals = block_synchronous_signals();
>> -
>> -    ret = qemu_signalfd_init(blocked_signals);
>> +    ret = qemu_signalfd_init();
>>       if (ret) {
>>           return ret;
>>       }
>> -#endif

iothread:

>>   int qemu_init_main_loop(void)
>>   {
>>       int ret;
>> -    sigset_t blocked_signals;
>>
>>       qemu_init_sigbus();
>>
>> -    blocked_signals = block_io_signals();
>> -
>> -    ret = qemu_signalfd_init(blocked_signals);
>> +    ret = qemu_signalfd_init();
>>       if (ret) {
>>           return ret;
>>       }

Paolo

On 2011-02-21 11:20, Paolo Bonzini wrote:
> On 02/21/2011 11:12 AM, Jan Kiszka wrote:
>> On 2011-02-21 09:43, Paolo Bonzini wrote:
>>> This makes it easier to add a Win32 stub.  The patch does that, too.
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>> ---
>>>   cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
>>>   1 files changed, 36 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 1b6893d..5bb95d8 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -346,11 +346,37 @@ static void sigfd_handler(void *opaque)
>>>       }
>>>   }
>>>
>>> -static int qemu_signalfd_init(sigset_t mask)
>>> +static int qemu_signalfd_init(void)
>>>   {
>>>       int sigfd;
>>> +    sigset_t set;
>>>
>>> -    sigfd = qemu_signalfd(&mask);
>>> +#ifdef CONFIG_IOTHREAD
>>> +    /* SIGUSR2 used by posix-aio-compat.c */
>>> +    sigemptyset(&set);
>>> +    sigaddset(&set, SIGUSR2);
>>> +    pthread_sigmask(SIG_UNBLOCK,&set, NULL);
>>
>> These lines are actually not related to signalfd setup. So you may want
>> to rename this function or move everything that does not block signals
>> and then pass the very same set to qemu_signalfd elsewhere.
> 
> Ok, I'll just rename qemu_signalfd_init to qemu_init_iothread_signals.
> 
>> For consistency reasons, you should also refactor the !IOTHREAD case.
> 
> I think I did?

Yeah, I must have missed that qemu_signalfd_init was already refactored
by whoever :) to be independent of CONFIG_IOTHREAD.

Jan