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

[Michael S. Tsirkin]

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.

-- 
MST