[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initi
From: |
Marcel Apfelbaum |
Subject: |
Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization |
Date: |
Mon, 10 Mar 2014 17:21:17 +0200 |
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?
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.
- I am not sure how gtester formatting options can helps us here, because
we *will* get an assert (after the first patch), but it would be in the
qtestlib
which is not the desired place. (it is not a qtestlib bug)
Thanks,
Marcel
>
> Stefan
>