[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks |
Date: |
Thu, 24 May 2018 21:15:49 +0300 |
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
[Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks, Michael S. Tsirkin, 2018/05/24
Re: [Qemu-devel] [PATCH] libqtest: fail if child coredumps, Peter Maydell, 2018/05/24