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