From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN5dQ-0000Zf-7W for qemu-devel@nongnu.org; Mon, 10 Mar 2014 15:13:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WN5dK-0003r5-7T for qemu-devel@nongnu.org; Mon, 10 Mar 2014 15:13:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN5dJ-0003qv-Vd for qemu-devel@nongnu.org; Mon, 10 Mar 2014 15:13:06 -0400 Date: Mon, 10 Mar 2014 20:13:01 +0100 From: Stefan Hajnoczi Message-ID: <20140310191301.GB12263@stefanha-thinkpad.redhat.com> References: <1394453534-24334-1-git-send-email-marcel.a@redhat.com> <20140310150259.GB32400@stefanha-thinkpad.redhat.com> <1394464877.3981.21.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1394464877.3981.21.camel@localhost.localdomain> Subject: Re: [Qemu-devel] [PATCH 0/2] tests: Fix possible deadlock in qtest initialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: Stefan Hajnoczi , armbru@redhat.com, afaerber@suse.de, aliguori@amazon.com, qemu-devel@nongnu.org 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 " 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