Re: [Qemu-devel] [PULL 21/31] block: Avoid bs->blk in bdrv_next()

[Kevin Wolf]

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 <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > ---
> >  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