On Mon, Mar 10, 2014 at 05:21:17PM +0200, Marcel Apfelbaum wrote:
> On Mon, 2014-03-10 at 16:02 +0100, Stefan Hajnoczi wrote:
> > On Mon, Mar 10, 2014 at 02:12:12PM +0200, Marcel Apfelbaum wrote:
> > > 'socket_accept' waits for Qemu to init its unix socket.
> > > If Qemu encounters an error during command line parsing,
> > > it can exit before initializing the communication channel.
> > > It gets worse as the make check-qtest-* gets stuck without
> > > notifying which test exactly has problems, so debugging can
> > > be a challenge.
> > >
> > > The solution has two parts:
> > > - Use a timeout for the socket.
> > > - Expose a qtest_state_valid that checks that the connections
> > > with Qemu are OK.
> >
> > See below why I think qtest_state_valid() is unnecessary as a libqtest.h
> > API.
> >
> > > Asserting qtest_state_valid in each test after qtest_init
> > > is a must, as we need to trace which test failed.
> >
> > Inability to tell which qtest failed is a Makefile problem. The
> > solution is not to move all asserts to the outer-most level just so the
> > error message includes the test name.
> >
> > Either we need to invoke gtester separately for each test - that way the
> > Makefile can print "TEST <name>" for each binary. Or maybe gtester has
> > options for formatting output better.
> Hi Stefan,
> Thanks for the review.
>
> I am more concerned of PATCH 1/2, because it is a blocker for another series
> I am working on.
> I can resend only the first one which adds socket timeout and leaves the
> original assert.
> Would you be OK with this?
Yes, that sounds fine. I posted comments on Patch 1 about ensuring the sockets
are unlinked before we fail (to prevent leaking the UNIX domain socket nodes in
the file system) - it does involve moving the assert to qtest_init().
> Now regarding the issue you brought up (less important):
> - Tweaking the Makefile to run each qtest separately and not all tests per
> arch
> is a viable solution, however I am not familiar with the makefile magic and
> it will take me a lot of time to get into it.
The following produces per-test output. This means you'll know which test
failed:
diff --git a/tests/Makefile b/tests/Makefile
index b17d41e..a8405c8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -273,7 +273,7 @@ check-help:
@echo "changed with variable GTESTER_OPTIONS."
SPEED = quick
-GTESTER_OPTIONS = -k $(if $(V),--verbose,-q)
+GTESTER_OPTIONS = -k #$(if $(V),--verbose,-q)
GCOV_OPTIONS = -n $(if $(V),-f,)
# gtester tests, possibly with verbose output
