On Thu, 10/01 15:13, Kevin Wolf wrote: > This simplifies the code somewhat, especially when dropping whole > backing file subchains. > > The exception is the mirroring code that does adventurous things with > bdrv_swap() and in order to keep it working, I had to duplicate most of > bdrv_set_backing_hd() locally. We'll get rid again of this ugliness > shortly. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > block.c | 35 ++++++++++++++--------------------- > block/mirror.c | 16 ++++++++++++---- > block/stream.c | 30 +----------------------------- > block/vvfat.c | 6 +++++- > include/block/block.h | 1 + > 5 files changed, 33 insertions(+), 55 deletions(-) > > diff --git a/block.c b/block.c > index 20a3c5c..c4dcf81 100644 > --- a/block.c > +++ b/block.c > @@ -1094,7 +1094,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState > *parent_bs, > return child; > } > > -static void bdrv_detach_child(BdrvChild *child) > +void bdrv_detach_child(BdrvChild *child) > { > QLIST_REMOVE(child, next); > g_free(child); > @@ -1112,13 +1112,20 @@ void bdrv_unref_child(BlockDriverState *parent, > BdrvChild *child) > bdrv_unref(child_bs); > } > > +/* > + * Sets the backing file link of a BDS. A new reference is created; callers > + * which don't need their own reference any more must call bdrv_unref(). > + */
Should we document the reference to bs->backing->bs as well? > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > { > + if (backing_hd) { > + bdrv_ref(backing_hd); > + } > > if (bs->backing) { > assert(bs->backing_blocker); > bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker); > - bdrv_detach_child(bs->backing); > + bdrv_unref_child(bs, bs->backing); > } else if (backing_hd) { > error_setg(&bs->backing_blocker, > "node is used as backing hd of '%s'", > @@ -1214,7 +1221,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict > *options, Error **errp) > goto free_exit; > } > > + /* Hook up the backing file link; drop our reference, bs owns the > + * backing_hd reference now */ > bdrv_set_backing_hd(bs, backing_hd); > + bdrv_unref(backing_hd); > > free_exit: > g_free(backing_filename); > @@ -1891,11 +1901,7 @@ void bdrv_close(BlockDriverState *bs) > bs->drv->bdrv_close(bs); > bs->drv = NULL; > > - if (bs->backing) { > - BlockDriverState *backing_hd = bs->backing->bs; > - bdrv_set_backing_hd(bs, NULL); > - bdrv_unref(backing_hd); > - } > + bdrv_set_backing_hd(bs, NULL); > > if (bs->file != NULL) { > bdrv_unref_child(bs, bs->file); > @@ -2428,12 +2434,9 @@ int bdrv_drop_intermediate(BlockDriverState *active, > BlockDriverState *top, > BlockDriverState *intermediate; > BlockDriverState *base_bs = NULL; > BlockDriverState *new_top_bs = NULL; > - BlkIntermediateStates *intermediate_state, *next; > + BlkIntermediateStates *intermediate_state; > int ret = -EIO; > > - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; > - QSIMPLEQ_INIT(&states_to_delete); > - > if (!top->drv || !base->drv) { > goto exit; > } > @@ -2460,7 +2463,6 @@ int bdrv_drop_intermediate(BlockDriverState *active, > BlockDriverState *top, > while (intermediate) { > intermediate_state = g_new0(BlkIntermediateStates, 1); > intermediate_state->bs = intermediate; > - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry); > > if (backing_bs(intermediate) == base) { > base_bs = backing_bs(intermediate); > @@ -2483,17 +2485,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, > BlockDriverState *top, > } > bdrv_set_backing_hd(new_top_bs, base_bs); > > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, > next) { > - /* so that bdrv_close() does not recursively close the chain */ > - bdrv_set_backing_hd(intermediate_state->bs, NULL); > - bdrv_unref(intermediate_state->bs); > - } > ret = 0; > - > exit: > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, > next) { > - g_free(intermediate_state); > - } > return ret; > } > > diff --git a/block/mirror.c b/block/mirror.c > index 5bb3e36..911c432 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -370,10 +370,18 @@ static void mirror_exit(BlockJob *job, void *opaque) > bdrv_swap(s->target, to_replace); > if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { > /* drop the bs loop chain formed by the swap: break the loop then > - * trigger the unref from the top one */ > - BlockDriverState *p = backing_bs(s->base); > - bdrv_set_backing_hd(s->base, NULL); > - bdrv_unref(p); > + * trigger the unref */ > + /* FIXME This duplicates bdrv_set_backing_hd(), except for the > + * actual detach/unref so that the loop can be broken. When > + * bdrv_swap() gets replaced, this will become sane again. */ > + BlockDriverState *backing = s->base->backing->bs; > + assert(s->base->backing_blocker); > + bdrv_op_unblock_all(backing, s->base->backing_blocker); > + error_free(s->base->backing_blocker); > + s->base->backing_blocker = NULL; > + bdrv_detach_child(s->base->backing); > + s->base->backing = NULL; > + bdrv_unref(backing); > } > } > if (s->to_replace) { > diff --git a/block/stream.c b/block/stream.c > index ba535c5..3f64fa2 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -52,34 +52,6 @@ static int coroutine_fn stream_populate(BlockDriverState > *bs, > return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov); > } > > -static void close_unused_images(BlockDriverState *top, BlockDriverState > *base, > - const char *base_id) > -{ > - BlockDriverState *intermediate; > - intermediate = backing_bs(top); > - > - /* Must assign before bdrv_delete() to prevent traversing dangling > pointer > - * while we delete backing image instances. > - */ > - bdrv_set_backing_hd(top, base); > - > - while (intermediate) { > - BlockDriverState *unused; > - > - /* reached base */ > - if (intermediate == base) { > - break; > - } > - > - unused = intermediate; > - intermediate = backing_bs(intermediate); > - bdrv_set_backing_hd(unused, NULL); > - bdrv_unref(unused); > - } > - > - bdrv_refresh_limits(top, NULL); > -} > - > typedef struct { > int ret; > bool reached_end; > @@ -101,7 +73,7 @@ static void stream_complete(BlockJob *job, void *opaque) > } > } > data->ret = bdrv_change_backing_file(job->bs, base_id, base_fmt); > - close_unused_images(job->bs, base, base_id); > + bdrv_set_backing_hd(job->bs, base); > } > > g_free(s->backing_file_str); > diff --git a/block/vvfat.c b/block/vvfat.c > index 7c4b0f5..b41055a 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -2923,6 +2923,7 @@ static BlockDriver vvfat_write_target = { > static int enable_write_target(BDRVVVFATState *s, Error **errp) > { > BlockDriver *bdrv_qcow = NULL; > + BlockDriverState *backing; > QemuOpts *opts = NULL; > int ret; > int size = sector2cluster(s, s->sector_count); > @@ -2971,7 +2972,10 @@ static int enable_write_target(BDRVVVFATState *s, > Error **errp) > unlink(s->qcow_filename); > #endif > > - bdrv_set_backing_hd(s->bs, bdrv_new()); > + backing = bdrv_new(); > + bdrv_set_backing_hd(s->bs, backing); > + bdrv_unref(backing); > + > s->bs->backing->bs->drv = &vvfat_write_target; > s->bs->backing->bs->opaque = g_new(void *, 1); > *(void**)s->bs->backing->bs->opaque = s; > diff --git a/include/block/block.h b/include/block/block.h > index c6854a6..5d9092c 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -513,6 +513,7 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs); > void bdrv_ref(BlockDriverState *bs); > void bdrv_unref(BlockDriverState *bs); > void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child); > +void bdrv_detach_child(BdrvChild *child); > > bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); > void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); > -- > 1.8.3.1 > >