qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert
@ 2018-07-20 15:39 Peter Maydell
  2018-07-20 15:48 ` Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Peter Maydell @ 2018-07-20 15:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: patches, Michael S . Tsirkin

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: libqtest.c: kill_qemu() tried to terminate QEMU process but it dumped core with signal 11
ahci-test: tests/libqtest.c:118: kill_qemu: Assertion `0' failed.
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>
---
In particular, the travis test config that enables gprof
seems to (a) run into this every so often and (b) have the
really unhelpful assertion text quoted above:
 https://travis-ci.org/qemu/qemu/jobs/406192798

Maybe for 3.0 since it's only test code.

 tests/libqtest.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec44..99341e1b47d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -110,7 +110,13 @@ 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)) {
+                fprintf(stderr,
+                        "libqtest.c: kill_qemu() tried to terminate QEMU "
+                        "process but it dumped core with signal %d\n",
+                        WTERMSIG(wstatus));
+                assert(0);
+            }
         }
     }
 }
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH for-3.0] tests/libqtest: Improve kill_qemu() assert
@ 2018-07-23 18:47 Peter Maydell
  2018-07-23 18:59 ` Eric Blake
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2018-07-23 18:47 UTC (permalink / raw)
  To: qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé,
	Alex Bennée, Michael S . Tsirkin

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(-)

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();
+            }
         }
     }
 }
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-07-23 19:46 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-20 15:39 [Qemu-devel] [PATCH for-3.0 ?] tests/libqtest: Improve kill_qemu() assert Peter Maydell
2018-07-20 15:48 ` Philippe Mathieu-Daudé
2018-07-20 15:49   ` Peter Maydell
2018-07-20 16:14     ` Richard Henderson
2018-07-20 16:25       ` Peter Maydell
2018-07-20 16:36         ` Richard Henderson
2018-07-20 16:45           ` Peter Maydell
2018-07-20 17:28             ` Richard Henderson
2018-07-22 15:27 ` Michael S. Tsirkin
2018-07-23 10:59 ` Alex Bennée
  -- strict thread matches above, loose matches on Subject: below --
2018-07-23 18:47 [Qemu-devel] [PATCH for-3.0] " Peter Maydell
2018-07-23 18:59 ` Eric Blake
2018-07-23 19:02   ` Peter Maydell
2018-07-23 19:46     ` Eric Blake

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).