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

[Kevin Wolf]

Am 30.08.2012 20:47, schrieb Jeff Cody:
> Add bdrv_find_child(), and bdrv_delete_intermediate().
> 
> bdrv_find_child():  given 'bs' and the active (topmost) BDS of an image chain,
>                     find the image that is the immediate top of 'bs'
> 
> bdrv_delete_intermediate():
>                     Given 3 BDS (active, top, base), delete images above
>                     base up to and including top, and set base to be the
>                     parent of top's child node.
> 
>                     E.g., this converts:
> 
>                     bottom <- base <- intermediate <- top <- active
> 
>                     to
> 
>                     bottom <- base <- active
> 
>                     where top == active is permitted, although active
>                     will not be deleted.
> 
> Signed-off-by: Jeff Cody <address@hidden>

At first, when just reading the function name, I thought this would
actually delete the image file. Of course, it only removes it from the
backing file chain, but leaves the image file around. I don't have a
good suggestion, but if someone has a better name, I think we should
change it.

> ---
>  block.c | 142 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h |   5 ++-
>  2 files changed, 146 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 9470319..11e275c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      return ret;
>  }
>  
> +/* Finds the image layer immediately to the 'top' of bs.
> + *
> + * active is the current topmost image.
> + */

Most other function header comments in block.c have the /* on its own
line, so it would be nice to stay consistent.

Without looking at the code yet, I'm not quite sure if I understand this
correctly. Could use an example:

  base <- a <- b <- c <- d

bdrv_find_child(d, a) would return b? Why is it called bdrv_find_child
then, b is neither a (direct) child of d nor a.

> +BlockDriverState *bdrv_find_child(BlockDriverState *active,
> +                                  BlockDriverState *bs)
> +{
> +    BlockDriverState *child = NULL;
> +    BlockDriverState *intermediate;
> +
> +    /* if the active bs layer is the same as the new top, then there
> +     * is no image above the top, so it will be returned as the child
> +     */

"new top"?

And should this be mentioned in the header comment? Not sure how
generally useful this behaviour is. If it's only the right thing for the
only caller, maybe returning NULL and handling the case in the caller
would be better.

> +    if (active == bs) {
> +        child = active;
> +    } else {
> +        intermediate = active;
> +        while (intermediate->backing_hd) {
> +            if (intermediate->backing_hd == bs) {
> +                child = intermediate;
> +                break;
> +            }
> +            intermediate = intermediate->backing_hd;
> +        }
> +    }
> +
> +    return child;
> +}

So it returns NULL when bs isn't in the backing file chain of active.
Probably worth mentioning in the comment as well.

> +
> +typedef struct BlkIntermediateStates {
> +    BlockDriverState *bs;
> +    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> +} BlkIntermediateStates;
> +
> +
> +/* deletes images above 'base' up to and including 'top', and sets the image
> + * above 'top' to have base as its backing file.
> + *
> + * 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
> + *
> + * It is also allowed for top==active, except in that case active is not
> + * deleted:

Hm, makes the interface inconsistent. Shouldn't you be using top ==
intermediate and it would work without any special casing?

> + *
> + * base <- intermediate <- top
> + *
> + * becomes
> + *
> + * base <- top
> + */
> +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
> +                             BlockDriverState *base)
> +{
> +    BlockDriverState *intermediate;
> +    BlockDriverState *base_bs = NULL;
> +    BlockDriverState *new_top_bs = NULL;
> +    BlkIntermediateStates *intermediate_state, *next;
> +    int ret = -1;
> +
> +    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_child(active, top);

new_top_bs is the first BDS that should not be removed from the chain.

Why do we pass top and then search for new_top instead of passing
new_top directly?

> +
> +    /* 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;
> +    }
> +
> +    if (new_top_bs == NULL) {

Never reached, we segfault before. (How about unit tests for this
function, in the style of tests/check-q*.c?)

> +        /* we could not find the image above 'top', this is an error */
> +        goto exit;
> +    }
> +
> +    /* if the active and top image passed in are the same, then we
> +     * can't delete the active, so we start one below
> +     */
> +    intermediate = (active == top) ? active->backing_hd : top;

Aha. So intermediate is used to undo the special case. Now we're always
on the last image to be deleted.

This is equivalent to an unconditional new_top_bs->backing_hd.

> +
> +    /* 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;
> +        }
> +        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;
> +    }
> +
> +    /* 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 
> : "");

Requires that new_top_bs is opened r/w. Definitely worth documenting.

Kevin