From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAgVH-0006gQ-0G for qemu-devel@nongnu.org; Tue, 04 Feb 2014 08:57:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WAgVB-00070Y-Vr for qemu-devel@nongnu.org; Tue, 04 Feb 2014 08:57:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:19216) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAgVB-00070R-NM for qemu-devel@nongnu.org; Tue, 04 Feb 2014 08:57:25 -0500 Date: Tue, 4 Feb 2014 14:57:22 +0100 From: Kevin Wolf Message-ID: <20140204135722.GJ3384@dhcp-200-207.str.redhat.com> References: <1391464280-25627-1-git-send-email-benoit.canet@irqsave.net> <1391464280-25627-4-git-send-email-benoit.canet@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1391464280-25627-4-git-send-email-benoit.canet@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V15 03/13] quorum: Add quorum_aio_writev and its dependencies. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 03.02.2014 um 22:51 hat Beno=EEt Canet geschrieben: > From: Beno=EEt Canet >=20 > Signed-off-by: Benoit Canet > --- > block/quorum.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > 1 file changed, 104 insertions(+) Starting with writes before the driver can even open an image is a weird order to do things in. It also doesn't make the review any easier when you don't know how things are initialised. > diff --git a/block/quorum.c b/block/quorum.c > index 157efdf..81bffdd 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -64,11 +64,115 @@ struct QuorumAIOCB { > int vote_ret; > }; > =20 > +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb) > +{ > + QuorumAIOCB *acb =3D container_of(blockacb, QuorumAIOCB, common); > + BDRVQuorumState *s =3D acb->bqs; > + int i; > + > + /* cancel all callback */ "callbacks" > + for (i =3D 0; i < s->total; i++) { > + bdrv_aio_cancel(acb->aios[i].aiocb); > + } > +} Don't you want to free acb and similar cleanup? > + > +static AIOCBInfo quorum_aiocb_info =3D { > + .aiocb_size =3D sizeof(QuorumAIOCB), > + .cancel =3D quorum_aio_cancel, > +}; > + > +static void quorum_aio_finalize(QuorumAIOCB *acb) > +{ > + BDRVQuorumState *s =3D acb->bqs; block/quorum.c: In function 'quorum_aio_finalize': block/quorum.c:86:22: error: unused variable 's' [-Werror=3Dunused-variab= le] > + int ret =3D 0; > + > + acb->common.cb(acb->common.opaque, ret); > + if (acb->finished) { > + *acb->finished =3D true; > + } > + g_free(acb->aios); > + qemu_aio_release(acb); > +} > + > +static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s, > + BlockDriverState *bs, > + QEMUIOVector *qiov, > + uint64_t sector_num, > + int nb_sectors, > + BlockDriverCompletionFunc *cb, > + void *opaque) > +{ > + QuorumAIOCB *acb =3D qemu_aio_get(&quorum_aiocb_info, bs, cb, opaq= ue); > + int i; > + > + acb->bqs =3D s; Noticed it only here, but it's really in patch 2 (and should be in patch 1): What is acb->bqs good for? Isn't it always the same as acb->common.bs->opaque? > + acb->sector_num =3D sector_num; > + acb->nb_sectors =3D nb_sectors; > + acb->qiov =3D qiov; > + acb->aios =3D g_new0(QuorumSingleAIOCB, s->total); > + acb->count =3D 0; > + acb->success_count =3D 0; > + acb->finished =3D NULL; > + acb->is_read =3D false; > + acb->vote_ret =3D 0; > + > + for (i =3D 0; i < s->total; i++) { > + acb->aios[i].buf =3D NULL; > + acb->aios[i].ret =3D 0; > + acb->aios[i].parent =3D acb; > + } > > + > + return acb; > +} Kevin