Re: [Qemu-devel] [PATCH v2 1/7] block: add support functions for live commit, to find and delete images.

[Kevin Wolf]

Am 25.09.2012 18:29, schrieb Jeff Cody:
> Add bdrv_find_overlay(), and bdrv_drop_intermediate().
> 
> bdrv_find_overlay():  given 'bs' and the active (topmost) BDS of an image 
> chain,
>                     find the image that is the immediate top of 'bs'
> 
> bdrv_drop_intermediate():
>                     Given 3 BDS (active, top, base), drop images above
>                     base up to and including top, and set base to be the
>                     backing file of top's overlay node.
> 
>                     E.g., this converts:
> 
>                     bottom <- base <- intermediate <- top <- active
> 
>                     to
> 
>                     bottom <- base <- active
> 
> Signed-off-by: Jeff Cody <address@hidden>

> +/*
> + * Drops images above 'base' up to and including 'top', and sets the image
> + * above 'top' to have base as its backing file.
> + *
> + * Requires that the overlay to 'top' is opened r/w, so that the backing file
> + * information in 'bs' can be properly updated.
> + *
> + * E.g., this will convert the following chain:
> + * bottom <- base <- intermediate <- top <- active
> + *
> + * to
> + *
> + * bottom <- base <- active
> + *
> + * It is allowed for bottom==base, in which case it converts:
> + *
> + * base <- intermediate <- top <- active
> + *
> + * to
> + *
> + * base <- active

Compared to RFC v2 you deleted this part of the comment:

  + *
  + * It is also allowed for top==active, except in that case active is not
  + * deleted:

You describe the condition now as an error (which I think is what our
conclusion on IRC was), but now I think the following example must be
removed as well:

> + *
> + * base <- intermediate <- top
> + *
> + * becomes
> + *
> + * base <- top
> + *
> + * Error conditions:
> + *  if active == top, that is considered an error
> + *
> + */
> +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;
> +    }
> +
> +    new_top_bs = bdrv_find_overlay(active, top);
> +
> +    if (new_top_bs == NULL) {
> +        /* we could not find the image above 'top', this is an error */
> +        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 = new_top_bs->backing_hd;  /* this should be the same
> +                                               as 'top' */

Should? Is it the same or not? If yes, you can write it as an assert();
it no, the comment is only confusing.

(In fact it's obvious enough that I'd just go with intermediate = top;
without any comment)

Kevin