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 <kw...@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 <mre...@redhat.com>
[...]
signature.asc
Description: OpenPGP digital signature
