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

[Jeff Cody <jcody@redhat.com>]

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 <jcody@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 :)