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

[Jeff Cody]

On 09/07/2012 06:19 AM, Kevin Wolf wrote:
> 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 <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.
>>
>> 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?

Sure - since active == top is now an error, there is no reason for the
more complicated logic.  And at this point, the statement
(new_top_bs->backing_hd == top) should always be true.

> 
> Kevin
>