[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next()
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next() |
Date: |
Tue, 29 Mar 2016 20:50:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
[...]
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize, (continued)
- [Qemu-devel] [PATCH 5/9] block: Avoid BDS.blk in bdrv_next(), Kevin Wolf, 2016/03/22
- [Qemu-devel] [PATCH 6/9] block: Remove bdrv_move_feature_fields(), Kevin Wolf, 2016/03/22
- [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations, Kevin Wolf, 2016/03/22
- [Qemu-devel] [PATCH 8/9] block: Don't return throttling info in query-named-block-nodes, Kevin Wolf, 2016/03/22
- [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next(), Kevin Wolf, 2016/03/22
- Re: [Qemu-devel] [PATCH 7/9] block: Avoid bs->blk in bdrv_next(),
Max Reitz <=
- [Qemu-devel] [PATCH 4/9] block: Remove bdrv_aio_multiwrite(), Kevin Wolf, 2016/03/22
- [Qemu-devel] [PATCH 9/9] block: Remove BlockDriverState.blk, Kevin Wolf, 2016/03/22
- Re: [Qemu-devel] [PATCH 0/9] block: Remove BlockDriverState.blk, Paolo Bonzini, 2016/03/22