Re: [Qemu-block] [PATCH v2 08/16] block: Manage backing file references in bdrv_set_backing_hd()

[Alberto Garcia]

On Thu 01 Oct 2015 03:13:26 PM CEST, Kevin Wolf wrote:

> @@ -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);

The purpose of this while() loop in the original code was to make a list
of all the intermediate states whose references need to be dropped after
the bdrv_set_backing_hd() call. Since now bdrv_set_backing_hd() is the
one who manages the backing references, this list is no longer
necessary, and you are actually leaking the BlkIntermediateStates
objects here (see the rest of the patch below).

The loop is also useful to check that 'base' is indeed part of the
backing chain, so that we should keep.

> @@ -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;
>  }

Berto