Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization

[Stefan Hajnoczi]

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