| Message ID | 20180723184752.22150-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-3.0] tests/libqtest: Improve kill_qemu() assert | expand |
On 07/23/2018 01:47 PM, Peter Maydell wrote: > In kill_qemu() we have an assert that checks that the QEMU process > didn't dump core: > assert(!WCOREDUMP(wstatus)); > > Unfortunately the WCOREDUMP macro here means the resulting message > is not very easy to comprehend on at least some systems: > > ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__ (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i))) & 0x80)' failed. > > and it doesn't identify what signal the process took. > > Instead of using a raw assert, print the information in an > easier to understand way: > > /i386/ahci/sanity: tests/libqtest.c:119: kill_qemu() tried to terminate QEMU process but it dumped core with signal 11 (Segmentation fault) > Aborted (core dumped) > > (Of course, the really useful information would be why the QEMU > process dumped core in the first place, but we don't have that > by the time the test program has picked up the exit status.) Last time we bike-shedded this, I suggested that we fix things to call waitpid() in a loop, and that we just assert that exit status is 0: https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05610.html In other words, why are we special-casing death-by-coredump, when ALL non-zero exit status (whether or not a core dump was involved) is contrary to the assumptions of the testsuite?
On 23 July 2018 at 19:59, Eric Blake <eblake@redhat.com> wrote: > In other words, why are we special-casing death-by-coredump, when ALL > non-zero exit status (whether or not a core dump was involved) is contrary > to the assumptions of the testsuite? Because we're trying to get as much actual information as we have out into the logs, not merely "die so the test fails"... thanks -- PMM
On 07/23/2018 02:02 PM, Peter Maydell wrote: > On 23 July 2018 at 19:59, Eric Blake <eblake@redhat.com> wrote: >> In other words, why are we special-casing death-by-coredump, when ALL >> non-zero exit status (whether or not a core dump was involved) is contrary >> to the assumptions of the testsuite? > > Because we're trying to get as much actual information > as we have out into the logs, not merely "die so the test > fails"... Fair enough; so I posted a v2 based on your starting point: https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg04438.html
diff --git a/tests/libqtest.c b/tests/libqtest.c index 098af6aec44..bfc86a15f4b 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -110,7 +110,16 @@ static void kill_qemu(QTestState *s) pid = waitpid(s->qemu_pid, &wstatus, 0); if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) { - assert(!WCOREDUMP(wstatus)); + if (WCOREDUMP(wstatus)) { + int sig = WTERMSIG(wstatus); + const char *signame = strsignal(sig) ?: "unknown ???"; + + fprintf(stderr, + "%s:%d: kill_qemu() tried to terminate QEMU " + "process but it dumped core with signal %d (%s)\n", + __FILE__, __LINE__, sig, signame); + abort(); + } } } }
In kill_qemu() we have an assert that checks that the QEMU process didn't dump core: assert(!WCOREDUMP(wstatus)); Unfortunately the WCOREDUMP macro here means the resulting message is not very easy to comprehend on at least some systems: ahci-test: tests/libqtest.c:113: kill_qemu: Assertion `!(((__extension__ (((union { __typeof(wstatus) __in; int __i; }) { .__in = (wstatus) }).__i))) & 0x80)' failed. and it doesn't identify what signal the process took. Instead of using a raw assert, print the information in an easier to understand way: /i386/ahci/sanity: tests/libqtest.c:119: kill_qemu() tried to terminate QEMU process but it dumped core with signal 11 (Segmentation fault) Aborted (core dumped) (Of course, the really useful information would be why the QEMU process dumped core in the first place, but we don't have that by the time the test program has picked up the exit status.) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- changes v1->v2: addressed some of the bikeshedding with: * print file-and-line in the fprintf message, and then just abort(), rather than assert(0) * print the signal name via strsignal() as well tests/libqtest.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)