Re: [Qemu-block] [PATCH v4 1/3] block: include base when checking image chain for block allocation

[Vladimir Sementsov-Ogievskiy]

09.04.2019 17:18, Alberto Garcia wrote:
> On Mon 08 Apr 2019 08:22:19 PM CEST, Andrey Shinkevich wrote:
>>    * Return true if (a prefix of) the given range is allocated in any image
>> - * between BASE and TOP (inclusive).  BASE can be NULL to check if the given
>> + * between BASE and TOP (TOP included). To check the BASE image, set the
>> + * 'include_base' to 'true'. The BASE can be NULL to check if the given
>>    * offset is allocated in any image of the chain.  Return false otherwise,
>>    * or negative errno on failure.
> 
> I'm not a native speaker but that sounds a bit odd to me.
> 
> Alternative:
> 
>    * Return true if (a prefix of) the given range is allocated in any image
>    * between BASE and TOP (BASE is only included if include_base is set).
>    * BASE can be NULL to check if the given offset is allocated in any
>    * image of the chain.  Return false otherwise, or negative errno on
>    * failure.
> 
>> -    while (intermediate && intermediate != base) {
>> +    while (include_base || intermediate != base) {
>>           int64_t pnum_inter;
>>           int64_t size_inter;
>>   
>> @@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>               n = pnum_inter;
>>           }
>>   
>> +        if (intermediate == base) {
>> +            break;
>> +        }
>> +
>>           intermediate = backing_bs(intermediate);
> 
> I find that the new condition + the break make things a bit less
> readable. I think it would be simpler with something like this:
> 
>     BlockDriverState *stop_at = include_base ? backing_bs(base) : base;
> 
>     while (intermediate != stop_at) {
>        ...
>     }
> 

But in this way you return back dependence on base, which we don't freeze and 
which
may disappear on some iteration. We should not touch backing_bs(base) in any 
way.


-- 
Best regards,
Vladimir