On Wed, Jul 17, 2013 at 05:42:13PM +0800, Fam Zheng wrote:
> bdrv_drop_intermediate used a local list to iterate through backing
> chain and delete each BDS. It is simplified while adopting to refcount
> mechanism.
>
Hi Fam,
The reason for the local list is to keep the BDS deletion
transactional, so it can be rolled back in case of error (see below)
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> block.c | 71
> ++++++++++-------------------------------------------------------
> 1 file changed, 11 insertions(+), 60 deletions(-)
>
> diff --git a/block.c b/block.c
> index 57a3876..499de22 100644
> --- a/block.c
> +++ b/block.c
> @@ -2027,12 +2027,6 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState
> *active,
> return overlay;
> }
>
> -typedef struct BlkIntermediateStates {
> - BlockDriverState *bs;
> - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> -} BlkIntermediateStates;
> -
> -
> /*
> * Drops images above 'base' up to and including 'top', and sets the image
> * above 'top' to have base as its backing file.
> @@ -2062,15 +2056,9 @@ typedef struct BlkIntermediateStates {
> int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> BlockDriverState *base)
> {
> - BlockDriverState *intermediate;
> - BlockDriverState *base_bs = NULL;
> BlockDriverState *new_top_bs = NULL;
> - BlkIntermediateStates *intermediate_state, *next;
> int ret = -EIO;
>
> - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
> - QSIMPLEQ_INIT(&states_to_delete);
> -
> if (!top->drv || !base->drv) {
> goto exit;
> }
> @@ -2082,58 +2070,21 @@ int bdrv_drop_intermediate(BlockDriverState *active,
> BlockDriverState *top,
> goto exit;
> }
>
> - /* special case of new_top_bs->backing_hd already pointing to base -
> nothing
> - * to do, no intermediate images */
> - if (new_top_bs->backing_hd == base) {
> - ret = 0;
> - goto exit;
> - }
> -
> - intermediate = top;
> -
> - /* now we will go down through the list, and add each BDS we find
> - * into our deletion queue, until we hit the 'base'
> - */
> - while (intermediate) {
> - intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
> - intermediate_state->bs = intermediate;
> - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
> -
> - if (intermediate->backing_hd == base) {
> - base_bs = intermediate->backing_hd;
> - break;
> + while (new_top_bs->backing_hd && new_top_bs->backing_hd != base) {
> + BlockDriverState *backing = new_top_bs->backing_hd;
> + if (backing == NULL) {
> + goto exit;
If you simplify it until just a while loop that unrefs/deletes the BDS
inside the loop as you navigate the chain, then any error exit leaves
you in a bad state, with a potentially invalid chain. This is one
such error potential.
> }
> - intermediate = intermediate->backing_hd;
> - }
> - if (base_bs == NULL) {
> - /* something went wrong, we did not end at the base. safely
> - * unravel everything, and exit with error */
> - goto exit;
> + new_top_bs->backing_hd = backing->backing_hd;
> + /* break backing_hd chain before releasing bs, so we don't free all
> the
> + * way up the backing chain */
> + backing->backing_hd = NULL;
> + bdrv_unref(backing, false);
These two statements, which unlink this BDS from the chain, can't be
undone now, in case of error.
> }
>
> - /* success - we can delete the intermediate states, and link top->base */
> - ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
> - base_bs->drv ? base_bs->drv->format_name
> : "");
> - if (ret) {
> - goto exit;
> - }
> - if (new_top_bs->backing_hd) {
> - bdrv_unref(new_top_bs->backing_hd, false);
> - }
> - new_top_bs->backing_hd = base_bs;
> - bdrv_ref(base_bs, false);
> -
> - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry,
> next) {
> - /* so that bdrv_close() does not recursively close the chain */
> - intermediate_state->bs->backing_hd = NULL;
> - bdrv_delete(intermediate_state->bs);
> - }
The foreach loop over the list was placed such that there were no more
error paths; we were guaranteed at this point to have been able delete
and unchain each intermediate BDS.
> - ret = 0;
> -
> + ret = bdrv_change_backing_file(new_top_bs, base->filename,
> + base->drv ? base->drv->format_name : "");
This is effectively another error path that would cause problems, if
ret < 0.
> exit:
> - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry,
> next) {
> - g_free(intermediate_state);
> - }
> return ret;
> }
>
> --
> 1.8.3.2
>
>
