On 07/31/2012 01:34 PM, Eric Blake wrote:
> On 07/30/2012 11:16 PM, Jeff Cody wrote:
>> Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().
>>
>> bdrv_find_image(): given a filename and a BDS, find the image in the chain
>> that matches the passed filename.
>>
>> bdrv_find_base(): given a BDS, find the base image (parent-most image)
>>
>> 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.
>
> A diagram, as was in your cover letter, would help (either here, or
> better yet in the comments describing this function in place)...
>
Or even better, both :)
I'll add that in for v1.
>>
>> Signed-off-by: Jeff Cody <jc...@redhat.com>
>> ---
>> block.c | 136
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> block.h | 4 ++
>> 2 files changed, 139 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 522acd1..a3c8d33 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs)
>>
>> if (!drv)
>> return -ENOMEDIUM;
>> -
>> +
>> if (!bs->backing_hd) {
>> return -ENOTSUP;
>> }
>
> Spurious whitespace cleanup, since nothing else in this hunk belongs to
> you. Is that trailing whitespace present upstream, or was it introduced
> in one of the patches you based off of?
Likely a spurious cleanup - I had several trailing whitespaces in my
block.c changes, and scripts/checkpatch.pl warned me of those. I
cleaned them up, and I must have cleaned up an extra one with my regex.
>
>> @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>> return ret;
>> }
>>
>> +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.
>> + */
>> +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState
>> *top,
>> + BlockDriverState *base)
>
> ...that is, I think this would aid the reader:
>
> Converts:
>
> bottom <- base <- intermediate <- top <- active
>
> to
>
> bottom <- base <- active
>
> where top==active is permitted
I agree that is better. And, for clarity, bottom==base is permitted as
well.
>
>> @@ -3128,6 +3232,36 @@ BlockDriverState
>> *bdrv_find_backing_image(BlockDriverState *bs,
>> return NULL;
>> }
>>
>> +BlockDriverState *bdrv_find_image(BlockDriverState *bs,
>> + const char *filename)
>> +{
>> + if (!bs || !bs->drv) {
>> + return NULL;
>> + }
>> +
>> + if (strcmp(bs->filename, filename) == 0) {
>> + return bs;
>> + } else {
>> + return bdrv_find_image(bs->backing_hd, filename);
>
> Tail-recursive; are we worried enough about ever hitting stack overflow
> due to a really deep change to convert this into a while loop recursion
> instead? [Probably not]
Not too worried about it, because the chain should not be *that* long,
and also the block layer handles the chain in a similar fashion other
places, so we'll likely blow up in those places first :)
That said, when doing some automated live snapshot testing with an
obscene number of snapshots, I did manage to blow the stack (IIRC) in
the recursive open. That was with something like a chain of 1500
snapshots, which seems a bit excessive.
But, I agree with your point below:
>
>> + }
>> +}
>> +
>> +BlockDriverState *bdrv_find_base(BlockDriverState *bs)
>> +{
>> + BlockDriverState *curr_bs = NULL;
>> +
>> + if (!bs) {
>> + return NULL;
>> + }
>> +
>> + curr_bs = bs;
>> +
>> + while (curr_bs->backing_hd) {
>> + curr_bs = curr_bs->backing_hd;
>> + }
>
> Then again, here you did while-loop recursion, so using the same style
> in both functions might be worthwhile.
>
Yes - maybe that is a good reason to have bdrv_find_image() be a
while-loop (I based it off of bdrv_find_backing_image(), which
is why it was recursive). In general, I find recursive functions make
my brain hurt, so I tend to like while-loops better :)
