Am 20.05.2016 um 09:54 hat Paolo Bonzini geschrieben:
>
>
> 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 <kw...@redhat.com>
> > Reviewed-by: Max Reitz <mre...@redhat.com>
> > ---
> > 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.
Already posted a fix. I chose to keep the interface and free the
BdrvNextIterator inside bdrv_next(), when we return NULL after the last
element.
Kevin
