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 <jc...@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?
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
>
