[0/2] tests: Fix possible deadlock in qtest initialization

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:

On Mon, 2014-03-10 at 20:13 +0100, Stefan Hajnoczi wrote:
> 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().
Hi Stefan,
Thanks a lot for your help! I already sent V2.

> 
> > 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:
Yes, it works like a charm! I added it :)

Thanks,
Marcel
> 
> 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