Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Thomas Huth <thuth@redhat.com>]

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