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

[Paolo Bonzini <pbonzini@redhat.com>]

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 <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@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.

Thanks,

Paolo