Re: [PATCH] tests/qtest: kill off QEMU with SIGKILL when qtest exits abnormally

[Thomas Huth <thuth@redhat.com>]

On 13/05/2022 16.47, Thomas Huth wrote:
> On 13/05/2022 16.37, Daniel P. Berrangé wrote:
>> If a qtest program exits without calling qtest_quit(), then the
>> QEMU emulator process will remain running in the background forever.
>>
>> Unfortunately this scenario is exactly what will happen when a
>> g_assert() check triggers an abort().
>>
>> Prior to switching to use of 'meson test', this problem would
>> cause tap-driver.pl to hang forever. It was waiting for its
>> STDIN to report EOF, but that would never happen due to the
>> ophaned QEMU emulator processes keeping the pipe open forever.
>> Fortunately this doesn't happen with meson, but it is still
>> desirable to not leak QEMU processes when asserts fire.
>>
>> Using the Linux specific prctl(PR_SET_PDEATHSIG) syscall, we
>> can ensure that QEMU gets sent SIGKILL as soon as the controlling
>> qtest exits, despite being daemonized.
>>
>> Note, technically the death signal is sent when the *thread* that
>> called fork() exits. IOW, if you are calling qtest_init() in one
>> thread, letting that thread exit, and then expecting to run
>> qtest_quit() in a different thread, things are not going to work
>> out. Fortunately that is not a scenario that exists in qtests,
>> as pairs of qtest_init and qtest_quit are always called from the
>> same thread.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   tests/qtest/libqtest.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 228357f1ea..553e82e492 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -19,6 +19,9 @@
>>   #include <sys/socket.h>
>>   #include <sys/wait.h>
>>   #include <sys/un.h>
>> +#ifdef __linux__
>> +#include <sys/prctl.h>
>> +#endif /* __linux__ */
>>   #include "libqtest.h"
>>   #include "libqmp.h"
>> @@ -301,6 +304,21 @@ QTestState *qtest_init_without_qmp_handshake(const 
>> char *extra_args)
>>       s->expected_status = 0;
>>       s->qemu_pid = fork();
>>       if (s->qemu_pid == 0) {
>> +#ifdef __linux__
>> +        /*
>> +         * If the controlling qtest process exits without calling
>> +         * the qtest_quit() method, the QEMU processes will get
>> +         * orphaned and remain running forever in the background.
>> +         *
>> +         * Missing qtest_quit() calls are, unfortunately, exactly
>> +         * what happen when a g_assert() check triggers abort() in
>> +         * a failing test scenario.
>> +         *
>> +         * This PR_SET_PDEATHSIG setup will ensure QEMU will
>> +         * get terminated with SIGKILL.
>> +         */
>> +        prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
>> +#endif /* __linux__ */
>>           if (!g_setenv("QEMU_AUDIO_DRV", "none", true)) {
>>               exit(1);
>>           }
> 
> Would it make sense to install a signal handler for SIGABRT instead and make 
> sure that we tear down the QEMU instance there? ... that would then also 
> work for other non-Linux operating systems?

Wait, we're doing that already ... why doesn't it work for your case?

  Thomas