Re: [Qemu-devel] [PATCH v4 13/15] block stream: add support for partial streaming

[Kevin Wolf]

Am 06.01.2012 15:01, schrieb Stefan Hajnoczi:
> From: Marcelo Tosatti <address@hidden>
> 
> Add support for streaming data from an intermediate section of the
> image chain (see patch and documentation for details).
> 
> Signed-off-by: Marcelo Tosatti <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c        |   64 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h        |    4 +++
>  block/stream.c |   28 +++++++++++++++++++++---
>  block_int.h    |    3 +-
>  blockdev.c     |   11 ++++++---
>  5 files changed, 101 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9b688a0..d2143b1 100644
> --- a/block.c
> +++ b/block.c
> @@ -2263,6 +2263,70 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t 
> sector_num, int nb_sectors,
>      return data.ret;
>  }
>  
> +/*
> + * Given an image chain: [BASE] -> [INTER1] -> [INTER2] -> [TOP]
> + *
> + * Return true if the given sector is allocated in top or base.
> + * Return false if the given sector is allocated in intermediate images.

This description is inexact. A sector could be allocated both in base in
an intermediate image.

Also initially I thought that we not only need to check whether the
sector is allocated in BASE, but also in any parents of BASE. You don't
do this: Clusters that are completely unallocated all through the chain
are reported as allocated.

After reading all of the patch, I believe this provides the right
semantics: "Normal" image streaming would copy them into the topmost
file, but if you keep a backing file, you want to copy as little as
possible and keep using the backing file whenever possible.

So I suggest to fix the description rather than the implementation.

Maybe we should also rename the function. With this semantics it's not a
generic is_allocated function any more, but something quite specific to
streaming with a base file.

Kevin