From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56589) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1Zy-0006eC-4g for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:26:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS1Zp-0003O8-Ir for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:26:18 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:39624 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1Zp-0003Nw-8F for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:26:09 -0400 Date: Thu, 11 Sep 2014 12:25:12 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140911102511.GA8522@irqsave.net> References: <1410336832-22160-1-git-send-email-armbru@redhat.com> <1410336832-22160-7-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1410336832-22160-7-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 06/23] block: Eliminate bdrv_states, use block_next() instead List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, benoit.canet@irqsave.net, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com The Wednesday 10 Sep 2014 =E0 10:13:35 (+0200), Markus Armbruster wrote : > Signed-off-by: Markus Armbruster > --- > block.c | 43 +++++++++++++++++++--------------------= ---- > block/block-backend.c | 4 ++++ > include/block/block_int.h | 2 -- > 3 files changed, 23 insertions(+), 26 deletions(-) >=20 > diff --git a/block.c b/block.c > index a6c03da..89f9cf0 100644 > --- a/block.c > +++ b/block.c > @@ -24,6 +24,7 @@ > #include "config-host.h" > #include "qemu-common.h" > #include "trace.h" > +#include "sysemu/block-backend.h" > #include "block/block_int.h" > #include "block/blockjob.h" > #include "qemu/module.h" > @@ -90,9 +91,6 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque); > static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs, > int64_t sector_num, int nb_sectors, BdrvRequestFlags flags); > =20 > -static QTAILQ_HEAD(, BlockDriverState) bdrv_states =3D > - QTAILQ_HEAD_INITIALIZER(bdrv_states); > - > static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =3D > QTAILQ_HEAD_INITIALIZER(graph_bdrv_states); > =20 > @@ -355,9 +353,6 @@ BlockDriverState *bdrv_new_named(const char *device= _name, Error **errp) > bs =3D bdrv_new(); > =20 > pstrcpy(bs->device_name, sizeof(bs->device_name), device_name); > - if (device_name[0] !=3D '\0') { > - QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list); > - } > =20 > return bs; > } > @@ -1888,7 +1883,7 @@ void bdrv_close_all(void) > { > BlockDriverState *bs; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > =20 > aio_context_acquire(aio_context); > @@ -1939,7 +1934,7 @@ void bdrv_drain_all(void) > while (busy) { > busy =3D false; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > bool bs_busy; > =20 > @@ -1960,9 +1955,6 @@ void bdrv_drain_all(void) > Also, NULL terminate the device_name to prevent double remove */ > void bdrv_make_anon(BlockDriverState *bs) > { > - if (bs->device_name[0] !=3D '\0') { > - QTAILQ_REMOVE(&bdrv_states, bs, device_list); > - } > bs->device_name[0] =3D '\0'; > if (bs->node_name[0] !=3D '\0') { > QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list); > @@ -2016,10 +2008,9 @@ static void bdrv_move_feature_fields(BlockDriver= State *bs_dest, > /* job */ > bs_dest->job =3D bs_src->job; > =20 > - /* keep the same entry in bdrv_states */ > pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name), > bs_src->device_name); > - bs_dest->device_list =3D bs_src->device_list; > + > memcpy(bs_dest->op_blockers, bs_src->op_blockers, > sizeof(bs_dest->op_blockers)); > } > @@ -2363,7 +2354,7 @@ int bdrv_commit_all(void) > { > BlockDriverState *bs; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > =20 > aio_context_acquire(aio_context); > @@ -3807,7 +3798,7 @@ BlockDriverState *bdrv_find(const char *name) > { > BlockDriverState *bs; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > if (!strcmp(name, bs->device_name)) { > return bs; > } > @@ -3888,17 +3879,21 @@ bool bdrv_chain_contains(BlockDriverState *top,= BlockDriverState *base) > =20 > BlockDriverState *bdrv_next(BlockDriverState *bs) > { > - if (!bs) { > - return QTAILQ_FIRST(&bdrv_states); > - } > - return QTAILQ_NEXT(bs, device_list); > + BlockBackend *blk; > + > + for (blk =3D blk_next(bs ? bs->blk : NULL); > + blk && !blk_bs(blk); > + blk =3D blk_next(blk)) > + ; > + > + return blk ? blk_bs(blk) : NULL; I find ?: hard to read and I know at least one maintainer prefers simples ifs to it. BlockDriverState *bdrv_next(BlockDriverState *bs) { BlockBackend *blk =3D NULL; /* if a BDS is provided use it's block backend as a starting point */ if (bs) { blk =3D bl->blk; } /* looking for the next block backend having a BDS attached */ for (blk =3D blk_next(blk); blk && !blk_bs(blk); blk =3D blk_next(blk)) ; /* the search was successfull */ if (blk) { return blk_bs(blk); } return NULL; } I think getting rid of ?: spread the brain load sequentially by being les= s compact but anyway your code is correct so it's up to you. > } > =20 > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void= *opaque) > { > BlockDriverState *bs; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > it(opaque, bs); > } > } > @@ -3918,7 +3913,7 @@ int bdrv_flush_all(void) > BlockDriverState *bs; > int result =3D 0; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > int ret; > =20 > @@ -5065,7 +5060,7 @@ void bdrv_invalidate_cache_all(Error **errp) > BlockDriverState *bs; > Error *local_err =3D NULL; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > =20 > aio_context_acquire(aio_context); > @@ -5082,7 +5077,7 @@ void bdrv_clear_incoming_migration_all(void) > { > BlockDriverState *bs; > =20 > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > AioContext *aio_context =3D bdrv_get_aio_context(bs); > =20 > aio_context_acquire(aio_context); > @@ -5918,7 +5913,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *c= andidate) > BlockDriverState *bs; > =20 > /* walk down the bs forest recursively */ > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > + for (bs =3D bdrv_next(NULL); bs; bs =3D bdrv_next(bs)) { > bool perm; > =20 > /* try to recurse in this top level bs */ > diff --git a/block/block-backend.c b/block/block-backend.c > index ae51f7f..c0876fa 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -191,6 +191,10 @@ BlockDriverState *blk_detach_bs(BlockBackend *blk) > { > BlockDriverState *bs =3D blk->bs; > =20 > + /* > + * Must bdrv_unref() before zapping bs->blk, or else > + * bdrv_drain_all() will miss bs. > + */ > if (bs) { > bs->blk =3D NULL; > blk->bs =3D NULL; > diff --git a/include/block/block_int.h b/include/block/block_int.h > index a04c852..d5de08b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -396,8 +396,6 @@ struct BlockDriverState { > QTAILQ_ENTRY(BlockDriverState) node_list; > /* Device name is the name associated with the "drive" the guest s= ees */ > char device_name[32]; > - /* element of the list of "drives" the guest sees */ > - QTAILQ_ENTRY(BlockDriverState) device_list; > QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; > int refcnt; > =20 > --=20 > 1.9.3 >=20