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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

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)...

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

> @@ -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

> @@ -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]

> +    }
> +}
> +
> +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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]