From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=35836 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4MtX-0007Qg-Nk for qemu-devel@nongnu.org; Mon, 28 Mar 2011 20:34:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4MtW-0004tu-A9 for qemu-devel@nongnu.org; Mon, 28 Mar 2011 20:34:51 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:64779) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4MtW-0004to-61 for qemu-devel@nongnu.org; Mon, 28 Mar 2011 20:34:50 -0400 Received: by vws17 with SMTP id 17so3159582vws.4 for ; Mon, 28 Mar 2011 17:34:49 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: Feiran Zheng Date: Tue, 29 Mar 2011 08:34:29 +0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Anthony Perard , Kevin Wolf , "qemu-devel@nongnu.org" , Gerd Hoffmann Sorry for the confusing, I'll clean up it. On Tue, Mar 29, 2011 at 2:16 AM, Stefano Stabellini wrote: > On Mon, 28 Mar 2011, Feiran Zheng wrote: >> On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini >> wrote: >> > On Sun, 27 Mar 2011, Feiran Zheng wrote: >> >> Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio' >> >> won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq i= n >> >> the blkdev->inflight list and a leak. >> >> >> >> Signed-off-by: Feiran Zheng >> >> --- >> >> =C2=A0hw/xen_disk.c | =C2=A0 22 +++++++++++++++++----- >> >> =C2=A01 files changed, 17 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/hw/xen_disk.c b/hw/xen_disk.c >> >> index 445bf03..7940fab 100644 >> >> --- a/hw/xen_disk.c >> >> +++ b/hw/xen_disk.c >> >> @@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *i= oreq) >> >> =C2=A0 =C2=A0 =C2=A0int i, rc, len =3D 0; >> >> =C2=A0 =C2=A0 =C2=A0off_t pos; >> >> >> >> - =C2=A0 =C2=A0if (ioreq->req.nr_segments && ioreq_map(ioreq) =3D=3D = -1) >> >> - =C2=A0 =C2=A0 goto err; >> >> + =C2=A0 =C2=A0if (ioreq->req.nr_segments) { >> >> + =C2=A0 =C2=A0 if (ioreq_map(ioreq) =3D=3D -1) >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err_no_map; >> >> + =C2=A0 =C2=A0} >> >> =C2=A0 =C2=A0 =C2=A0if (ioreq->presync) >> >> =C2=A0 =C2=A0 =C2=A0 bdrv_flush(blkdev->bs); >> >> >> > >> > this change is just to make the code clearer and easier to read, right= ? >> >> I think so. >> >> > >> > >> >> @@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *io= req) >> >> =C2=A0 =C2=A0 =C2=A0return 0; >> >> >> >> =C2=A0err: >> >> + =C2=A0 =C2=A0ioreq_unmap(ioreq); >> >> +err_no_map: >> >> + =C2=A0 =C2=A0ioreq_finish(ioreq); >> >> =C2=A0 =C2=A0 =C2=A0ioreq->status =3D BLKIF_RSP_ERROR; >> >> =C2=A0 =C2=A0 =C2=A0return -1; >> >> =C2=A0} >> >> @@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *io= req) >> >> =C2=A0{ >> >> =C2=A0 =C2=A0 =C2=A0struct XenBlkDev *blkdev =3D ioreq->blkdev; >> >> >> >> - =C2=A0 =C2=A0if (ioreq->req.nr_segments && ioreq_map(ioreq) =3D=3D = -1) >> >> - =C2=A0 =C2=A0 goto err; >> >> + =C2=A0 =C2=A0if (ioreq->req.nr_segments) { >> >> + =C2=A0 =C2=A0 if (ioreq_map(ioreq) =3D=3D -1) >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto err_no_map; >> >> + =C2=A0 =C2=A0} >> >> >> >> =C2=A0 =C2=A0 =C2=A0ioreq->aio_inflight++; >> >> =C2=A0 =C2=A0 =C2=A0if (ioreq->presync) >> >> @@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *io= req) >> >> =C2=A0 =C2=A0 =C2=A0qemu_aio_complete(ioreq, 0); >> >> >> >> =C2=A0 =C2=A0 =C2=A0return 0; >> >> + >> >> +err_no_map: >> >> + =C2=A0 =C2=A0ioreq_finish(ioreq); >> >> + =C2=A0 =C2=A0ioreq->status =3D BLKIF_RSP_ERROR; >> >> + =C2=A0 =C2=A0return -1; >> >> >> > >> > why aren't you calling ioreq_unmap before ioreq_finish, like in the >> > previous case? >> > >> > >> > >> It seems not a must but if call qemu_aio_complete, the error info can >> be printed, I thought it may be helpful. > > I am not sure I understand what you mean here because in case of error > we don't call qemu_aio_complete at all in the err_no_map code path. --=20 Best regards! Fam Zheng