From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=40141 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OxiCm-000588-F5
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:22:57 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <stefanha@gmail.com>) id 1OxiCl-0003Kb-8g
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:22:56 -0400
Received: from mail-px0-f173.google.com ([209.85.212.173]:38552)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <stefanha@gmail.com>) id 1OxiCl-0003KO-4F
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:22:55 -0400
Received: by pxi12 with SMTP id 12so1312337pxi.4
	for <qemu-devel@nongnu.org>; Mon, 20 Sep 2010 08:22:53 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <4C97744F.50307@redhat.com>
References: <1283614225-23651-1-git-send-email-stefanha@linux.vnet.ibm.com>
	<1283614468-25230-1-git-send-email-stefanha@linux.vnet.ibm.com>
	<4C97744F.50307@redhat.com>
Date: Mon, 20 Sep 2010 16:22:53 +0100
Message-ID: <AANLkTimPfSv1QCDQTfnhMvy_usMzMDHDjUa59=fG7uR2@mail.gmail.com>
Subject: Re: [Qemu-devel] Re: [PATCH v3] blkverify: Add block driver for
	verifying I/O
From: Stefan Hajnoczi <stefanha@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>, qemu-devel@nongnu.org

Thanks for your comments Kevin!

I'd like to merge the overlapping I/O patch I sent today since
blkverify is not in mainline yet.  Are you okay with that or should I
keep them separate?

On Mon, Sep 20, 2010 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 04.09.2010 17:34, schrieb Stefan Hajnoczi:
>> +static void blkverify_aio_cancel(BlockDriverAIOCB *acb)
>> +{
>> + =A0 =A0bool done =3D false;
>> +
>> + =A0 =A0/* Allow the request to complete so the raw file and test file =
stay in
>> + =A0 =A0 * sync. =A0Hijack the callback (since the request is cancelled=
 anyway) to
>> + =A0 =A0 * wait for completion.
>> + =A0 =A0 */
>
> The approach of completing the request sounds okay, but I think you need
> to call the original callback if you let it complete.

Neither posix-aio-compat.c nor block/qcow2.c invoke the callback.  Am
I missing something?

>> + =A0 =A0acb->cb =3D blkverify_aio_cancel_cb;
>> + =A0 =A0acb->opaque =3D &done;
>> + =A0 =A0while (!done) {
>> + =A0 =A0 =A0 =A0qemu_aio_wait();
>> + =A0 =A0}
>
> qemu_aio_release(acb)?

Will fix.

>> + =A0 =A0/* Open the test file */
>> + =A0 =A0s->test_file =3D bdrv_new("");
>> + =A0 =A0ret =3D bdrv_open(s->test_file, filename, flags, NULL);
>> + =A0 =A0if (ret < 0) {
>> + =A0 =A0 =A0 =A0return ret;
>
> This leaks bs->file.

s->test_file needs bdrv_delete().  block.c:bdrv_open_common() calls
bdrv_delete(bs->file) on failure to close and delete bs->file, but we
don't need to rely on that.  I will fix both.

Stefan