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

[Kevin Wolf <kwolf@redhat.com>]

Am 06.09.2012 16:59, schrieb Jeff Cody:
> On 09/06/2012 09:23 AM, Kevin Wolf wrote:
>> 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 <jcody@redhat.com>
>>
>> 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.
> 
> Hmm, the naming seems consistent with bdrv_delete(), which does not
> actually delete the image files either (and, that is essentially what
> this does... calls bdrv_delete(), on the intermediate images).
> 
> However, here are some other name proposals:
> 
>    *  bdrv_disconnect_intermediate()
>    *  bdrv_drop_intermediate()
>    *  bdrv_shorten_chain()

bdrv_drop_intermediate() sounds good to me.

>>
>>> +
>>> +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?
>>
> 
> To remain consistent, maybe we should define it as an error if
> top==active, and return error in that case?  The caller can be
> responsible for checking for that - if the caller wants to merge down
> the active layer, there are additional steps to be taken anyway.

Yes, why not.

And we can always revisit when implementing the additional functionality.

>>> +        /* 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.

How about changing this to use the simpler unconditional version?

Kevin