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

[Stefan Hajnoczi]

On Thu, Jan 12, 2012 at 12:42 PM, Kevin Wolf <address@hidden> wrote:
> 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.

I have moved the function into block/stream.c and renamed it to just
is_allocated_base().  The description is updated.

This makes it clearer that it's a special-case is_allocated-like function.

Stefan