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

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 07:58:06PM +0200, Thomas Huth wrote:
> 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

I think we are better off defining it if it's not defined.
Will post an osdep patch.

-- 
MST