Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3239 bytes --]

On 22.03.2016 20:36, 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>
> ---
>  block.c                        | 34 +++++++++-----------------------
>  block/block-backend.c          | 44 +++++++++++++++++++++++++++---------------
>  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, 77 insertions(+), 66 deletions(-)

The connection with patch 5 is a bit funny. I see why you have two
patches for this issue (because the first one introduces
bdrv_has_blk()), but I think it would be better to clear up their
relationship in the first patch's commit message.

[...]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index fdd1727..b71b822 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -325,28 +325,40 @@ 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 {
> +    int phase;

A verbose enum would have been nicer.

>      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_new0(BdrvNextIterator, 1);
> +    }
> +
> +    if (it->phase == 0) {
> +        do {
> +            it->blk = blk_all_next(it->blk);
> +            *bs = it->blk ? blk_bs(it->blk) : NULL;
> +        } while (it->blk && *bs == NULL);

This will be interesting with multiple BBs per BDS. I guess we can do
something like only take the BDS if the BB is the first one in its
parent list.

> +
> +        if (*bs) {
> +            return it;

:-)

> +        }
> +        it->phase = 1;
>      }
>  
> +    /* Ignore all BDSs that are attached to a BlockBackend here; they have been
> +     * handled by the above block already */
>      do {
> -        blk = blk_all_next(blk);
> -    } while (blk && !blk->root);
> +        it->bs = bdrv_next_monitor_owned(it->bs);
> +        *bs = it->bs;
> +    } while (*bs && bdrv_has_blk(*bs));
>  
> -    return blk ? blk->root->bs : NULL;
> +    return *bs ? it : NULL;
>  }
>  
>  /*

Reviewed-by: Max Reitz <mreitz@redhat.com>

[...]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]