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

[Eric Blake]

On 05/24/2018 10:38 AM, 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
>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>
> Changes from v1:
> - drop SIGTERM as suggested by Eric

> +++ b/tests/libqtest.c
> @@ -110,7 +110,12 @@ static void kill_qemu(QTestState *s)
>           pid = waitpid(s->qemu_pid, &wstatus, 0);
>   
>           if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {

Wait a moment. If WIFSIGNALED() is true...

> +            /* Core dump is never OK */
>               assert(!WCOREDUMP(wstatus));
> +            /* Must exit normally */
> +            assert(WIFEXITED(wstatus));

...then WIFEXITED() is false.  This is bogus.

> +            /* If exited normally - check exit status */
> +            assert(!WIFEXITED(wstatus) || !WEXITSTATUS(wstatus));

And you have some redundancy - !WIFEXITED() is not possible if you just 
asserted WIFEXITED().

Better would be:

if (pid == s->qemu_pid) {
    /*
     * Since sending SIGTERM turns into a normal exit, we want to flag
     * any non-normal exit, whether or not it dumped core, as a test
     * failure (even if it was a SIGKILL from someone desperate to stop
     * the testsuite).
     */
    assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));
}

Also, since waitpid() can only return either s->qemu_pid or -1 as we 
aren't using WNOHANG, it may also be worth asserting that if pid == -1, 
we either have EAGAIN (but why aren't we looping in that case?) or ECHILD.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org