On 01/22/21 19:19, Eric Blake wrote:
> On 1/22/21 11:58 AM, Max Reitz wrote:
>
>>>> + if (!self) {
>>>> + /*
>>>> + * This SIGUSR2 came from an external source, not from
>>>> + * qemu_coroutine_new(), so perform the default action.
>>>> + */
>>>> + exit(0);
>>>> + }
>>>
>>> (2) exit() is generally unsafe to call in signal handlers.
>>>
>>> We could reason whether or not it is safe in this particular case (POSIX
>>> describes the exact conditions --
>>> <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03>),
>>>
>>> but it's much simpler to just call _exit().
>>>
>>>
>>> (3) "Performing the default action" would be slightly different from
>>> calling _exit(). When a process is terminated with a signal, the parent
>>> can distinguish that, when reaping the child. See waitpid() /
>>> WIFSIGNALED() / WTERMSIG(), versus WIFEXITED() / WEXITSTATUS().
>>>
>>> So for the "default action", we'd have to:
>>> - restore the SIGUSR2 handler to SIG_DFL, and
>>> - re-raise the signal for the thread, and
>>> - because the signal being handled is automatically blocked unless
>>> SA_NODEFER was set: unblock the signal for the thread.
>>>
>>> The pthread_sigmask() call, made for the last step, would be the one
>>> that would not return.
>>>
>>> *However*, all of this complexity is not really useful here. We don't
>>> really want to simulate being "forcefully terminated" by the unexpected
>>> (asynchronous) SIGUSR2. We just want to exit.
>>>
>>> Therefore, _exit() is fine. But, we should use an exit code different
>>> from 0, as this is definitely not a pristine exit from QEMU. I'm not
>>> sure if a convention exists for nonzero exit codes in QEMU; if not, then
>>> just pass EXIT_FAILURE to _exit().
>>
>> I’m fine with calling _exit(). I hope, Eric is, too (as long as the
>> comment no longer claims this were the default behavior).
>
> Using _exit(nonzero) is fine by me as long as the comment is accurate.
> There are signals like SIGINT where you really DO want to terminate by
> signal rather than using _exit(SIGINT+128), because shells can tell the
> difference [1]; but SIGUSR2 is not one of the signals where shells
> special-case their behavior.
>
> [1] https://www.cons.org/cracauer/sigint.html
Good point!
>
>>
>> Given that SIGUSR2 is not something that qemu is prepared to receive
>> from the outside, EXIT_FAILURE seems right to me. (Even abort() could
>> be justified, but I don’t think generating a core dump does any good here.)
>>
>> (As for qemu-specific exit code conventions, the only ones I know of are
>> for certain qemu-img subcommands. I’m sure you grepped, too, but I
>> can’t find anything for the system emulator.)
>>
>>> (4) Furthermore, please update the comment, because "perform the default
>>> action" is not precise.
>>
>> Sure, that’s definitely easier than to re-raise SIGUSR2.
>
> Works for me as well.
>
Thanks.
Laszlo
