From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3fGC-0000rw-G1 for qemu-devel@nongnu.org; Fri, 20 May 2016 03:54:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b3fG9-0004te-9U for qemu-devel@nongnu.org; Fri, 20 May 2016 03:54:16 -0400 Sender: Paolo Bonzini References: <1463671329-22655-1-git-send-email-kwolf@redhat.com> <1463671329-22655-22-git-send-email-kwolf@redhat.com> From: Paolo Bonzini Message-ID: <6e34bd03-65d6-ceab-be3b-0e24450ed006@redhat.com> Date: Fri, 20 May 2016 09:54:09 +0200 MIME-Version: 1.0 In-Reply-To: <1463671329-22655-22-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 21/31] block: Avoid bs->blk in bdrv_next() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org On 19/05/2016 17:21, Kevin Wolf wrote: > We need to introduce a separate BdrvNextIterator struct that can keep > more state than just the current BDS in order to avoid using the bs->blk > pointer. > > Signed-off-by: Kevin Wolf > Reviewed-by: Max Reitz > --- > block.c | 40 +++++++---------------- > block/block-backend.c | 72 +++++++++++++++++++++++++++++------------- > block/io.c | 13 ++++---- > block/snapshot.c | 30 +++++++++++------- > blockdev.c | 3 +- > include/block/block.h | 3 +- > include/sysemu/block-backend.h | 1 - > migration/block.c | 4 ++- > monitor.c | 6 ++-- > qmp.c | 5 ++- > 10 files changed, 102 insertions(+), 75 deletions(-) > > diff --git a/block.c b/block.c > index fd4cf81..91bf431 100644 > --- a/block.c > +++ b/block.c > @@ -2870,25 +2870,6 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs) > return QTAILQ_NEXT(bs, node_list); > } > > -/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by > - * the monitor or attached to a BlockBackend */ > -BlockDriverState *bdrv_next(BlockDriverState *bs) > -{ > - if (!bs || bs->blk) { > - bs = blk_next_root_bs(bs); > - if (bs) { > - return bs; > - } > - } > - > - /* Ignore all BDSs that are attached to a BlockBackend here; they have been > - * handled by the above block already */ > - do { > - bs = bdrv_next_monitor_owned(bs); > - } while (bs && bs->blk); > - return bs; > -} > - > const char *bdrv_get_node_name(const BlockDriverState *bs) > { > return bs->node_name; > @@ -3220,10 +3201,11 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > void bdrv_invalidate_cache_all(Error **errp) > { > - BlockDriverState *bs = NULL; > + BlockDriverState *bs; > Error *local_err = NULL; > + BdrvNextIterator *it = NULL; > > - while ((bs = bdrv_next(bs)) != NULL) { > + while ((it = bdrv_next(it, &bs)) != NULL) { > AioContext *aio_context = bdrv_get_aio_context(bs); > > aio_context_acquire(aio_context); > @@ -3265,10 +3247,11 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs, > int bdrv_inactivate_all(void) > { > BlockDriverState *bs = NULL; > + BdrvNextIterator *it = NULL; > int ret = 0; > int pass; > > - while ((bs = bdrv_next(bs)) != NULL) { > + while ((it = bdrv_next(it, &bs)) != NULL) { > aio_context_acquire(bdrv_get_aio_context(bs)); > } > > @@ -3277,8 +3260,8 @@ int bdrv_inactivate_all(void) > * the second pass sets the BDRV_O_INACTIVE flag so that no further write > * is allowed. */ > for (pass = 0; pass < 2; pass++) { > - bs = NULL; > - while ((bs = bdrv_next(bs)) != NULL) { > + it = NULL; > + while ((it = bdrv_next(it, &bs)) != NULL) { > ret = bdrv_inactivate_recurse(bs, pass); > if (ret < 0) { > goto out; > @@ -3287,8 +3270,8 @@ int bdrv_inactivate_all(void) > } > > out: > - bs = NULL; > - while ((bs = bdrv_next(bs)) != NULL) { > + it = NULL; > + while ((it = bdrv_next(it, &bs)) != NULL) { > aio_context_release(bdrv_get_aio_context(bs)); > } > > @@ -3781,10 +3764,11 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, > */ > bool bdrv_is_first_non_filter(BlockDriverState *candidate) > { > - BlockDriverState *bs = NULL; > + BlockDriverState *bs; > + BdrvNextIterator *it = NULL; > > /* walk down the bs forest recursively */ > - while ((bs = bdrv_next(bs)) != NULL) { > + while ((it = bdrv_next(it, &bs)) != NULL) { > bool perm; > > /* try to recurse in this top level bs */ > diff --git a/block/block-backend.c b/block/block-backend.c > index 9dcac97..7f2eeb0 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -75,6 +75,7 @@ static const AIOCBInfo block_backend_aiocb_info = { > }; > > static void drive_info_del(DriveInfo *dinfo); > +static BlockBackend *bdrv_first_blk(BlockDriverState *bs); > > /* All BlockBackends */ > static QTAILQ_HEAD(, BlockBackend) block_backends = > @@ -286,28 +287,50 @@ BlockBackend *blk_next(BlockBackend *blk) > : QTAILQ_FIRST(&monitor_block_backends); > } > > -/* > - * Iterates over all BlockDriverStates which are attached to a BlockBackend. > - * This function is for use by bdrv_next(). > - * > - * @bs must be NULL or a BDS that is attached to a BB. > - */ > -BlockDriverState *blk_next_root_bs(BlockDriverState *bs) > -{ > +struct BdrvNextIterator { > + enum { > + BDRV_NEXT_BACKEND_ROOTS, > + BDRV_NEXT_MONITOR_OWNED, > + } phase; > BlockBackend *blk; > + BlockDriverState *bs; > +}; > > - if (bs) { > - assert(bs->blk); > - blk = bs->blk; > - } else { > - blk = NULL; > +/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by > + * the monitor or attached to a BlockBackend */ > +BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs) > +{ > + if (!it) { > + it = g_new(BdrvNextIterator, 1); Hmm, who frees it? (Especially if the caller exits the loop prematurely, which means you cannot just free it before returning NULL). I think it's better to: - allocate the iterator on the stack and make bdrv_next return a BDS * - and add a bdrv_first function that does this: > + *it = (BdrvNextIterator) { > + .phase = BDRV_NEXT_BACKEND_ROOTS, > + }; and then returns bdrv_next(it); - if desirable add a macro that abstracts the calls to bdrv_first and bdrv_next. Thanks, Paolo