From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43994) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjy4-0002bg-H5 for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:45:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCjy1-0007NC-Cx for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:45:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33235) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCjy1-0007N7-7G for qemu-devel@nongnu.org; Tue, 14 Jun 2016 04:45:01 -0400 Date: Tue, 14 Jun 2016 10:44:58 +0200 From: Kevin Wolf Message-ID: <20160614084458.GD4916@noname.str.redhat.com> References: <1465378757-15271-1-git-send-email-den@openvz.org> <20160608112357.GD5324@noname.str.redhat.com> <575FC184.4090903@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <575FC184.4090903@openvz.org> Subject: Re: [Qemu-devel] [PATCH 1/1] hmp: acquire aio_context in hmp_qemu_io List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Vladimir Sementsov-Ogievskiy , Paolo Bonzini Am 14.06.2016 um 10:34 hat Denis V. Lunev geschrieben: > On 06/08/2016 02:23 PM, Kevin Wolf wrote: > >Am 08.06.2016 um 11:39 hat Denis V. Lunev geschrieben: > >>From: Vladimir Sementsov-Ogievskiy > >> > >>Acquire aio context before run command, this is mandatory for unit tests. > >> > >>Signed-off-by: Vladimir Sementsov-Ogievskiy > >>Signed-off-by: Denis V. Lunev > >>CC: Kevin Wolf > >>CC: Paolo Bonzini > >Looks right to me, but why "mandatory for unit tests"? Does this fix an > >observable bug in unit tests? If so, we should be more specific in the > >commit message. > > > >But in fact, I would only expect it to make a difference for dataplane > >and I don't think we have test cases that use both the 'qemu-io' HMP > >command and dataplane. > > > >Kevin > the problem is that it is usually difficult to understand that > the problem is in locking and find where the locking is missed. > Here we do have a place, which is used by unit testing, with > a locking missed. > > May be now tests are not failing, but IMHO these tests MUST > be fixed to accommodate future changes and thus this patch > is mandatory for them. I was never objecting to the patch, but just curious about the details of a possible failure you were seeing because I didn't see how to hit it in practice without dataplane. Sorry for forgetting about the patch, I've applied it now. Kevin