[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks
From: |
Thomas Huth |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks |
Date: |
Thu, 24 May 2018 19:58:06 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 24.05.2018 19:33, Michael S. Tsirkin wrote:
> On Thu, May 24, 2018 at 07:26:16PM +0200, Thomas Huth wrote:
>> On 24.05.2018 18:11, Michael S. Tsirkin wrote:
>>> Add more checks on how did QEMU exit.
>>>
>>> Legal ways to exit right now:
>>> - exit(0) or return from main
>>> - kill(SIGTERM) - sent by testing infrastructure
>>>
>>> Anything else is illegal.
>> [...]
>>> - if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
>>> + /* waitpid returns child PID on success */
>>> + assert(pid == s->qemu_pid);
>>> +
>>> + /* If exited on signal - check the reason: core dump is never OK */
>>> + if (WIFSIGNALED(wstatus)) {
>>> assert(!WCOREDUMP(wstatus));
>>> }
>>> + /* If exited normally - check exit status */
>>> + if (WIFEXITED(wstatus)) {
>>> + assert(!WEXITSTATUS(wstatus));
>>> + }
>>> + /* Valid ways to exit: right now only return from main or exit */
>>> + assert(WIFEXITED(wstatus));
>>> }
>>> }
>>
>> It's strange that you always get WIFEXITED(wstatus) == true here, even
>> if QEMU has been terminated by SIGTERM? I assume that's due to the fact
>> that QEMU intercepts SIGTERM and terminates via exit() instead?
>
> Right now, yes. This can of course change, so it's not
> a good idea hard-coding this assumption to deep
> in the code, imho.
>
>> So I
>> think you could simply replace the last three asserts with:
>>
>> assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));
>>
>> Thomas
>
> I could but they would be harder to debug.
>
> If I see
> "assertion failed: !WCOREDUMP(wstatus)"
> then that is very readable.
>
> If I just see
> "assertion failed: WIFEXITED(wstatus) && !WEXITSTATUS(wstatus)"
> then I just know something went wrong.
Then simply use:
assert(WIFEXITED(wstatus));
assert(!WEXITSTATUS(wstatus));
If QEMU exited due to a signal, you'll see the first assert, and if it
returned a non-zero exit code, you'll see the second assert. That's all
you really need to know here, I think.
I don't think that you gain anything by checking WCOREDUMP() here. And
according to the man-page of waitpid:
This macro is not specified in POSIX.1-2001 and is not
available on some UNIX implementations (e.g., AIX, SunOS).
Only use this enclosed in #ifdef WCOREDUMP ... #endif.
So if you insist on using that macro, you might need to add some #ifdef
code around it, I think.
Thomas
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, (continued)
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Eric Blake, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Eric Blake, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Thomas Huth, 2018/05/24
- Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
[Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
Re: [Qemu-devel] [PATCH] libqtest: fail if child coredumps, Peter Maydell, 2018/05/24