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

[Eric Blake <eblake@redhat.com>]

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 <mst@redhat.com>
> ---
> 
> 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