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?
Kevin
