Re: [Qemu-devel] [PATCH for-4.0 1/3] block: continue until base is found in bdrv_freeze_backing_chain() et al

[Vladimir Sementsov-Ogievskiy]

28.03.2019 13:27, Vladimir Sementsov-Ogievskiy wrote:
> 26.03.2019 20:07, Alberto Garcia wrote:
>> All three functions that handle the BdrvChild.frozen attribute walk
>> the backing chain from 'bs' to 'base' and stop either when 'base' is
>> found or at the end of the chain if 'base' is NULL.
>>
>> However if 'base' is not found then the functions return without
>> errors as if it was NULL.
>>
>> This is wrong: if the caller passed an incorrect parameter that means
>> that there is a bug in the code.
>>
>> Signed-off-by: Alberto Garcia <address@hidden>
> 
> interesting that bdrv_is_allocated_above has the same flaw. Could you fix it 
> too?

However bdrv_is_allocated_above is differs from your functions, as it may 
yield.. And
graph may change while it is running.. Shouldn't we freeze backing chain every 
time
we want to call bdrv_is_allocated_above?

> 
>> ---
>>   block.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0a93ee9ac8..3050854528 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4218,14 +4218,15 @@ BlockDriverState *bdrv_find_base(BlockDriverState 
>> *bs)
>>   /*
>>    * Return true if at least one of the backing links between @bs and
>>    * @base is frozen. @errp is set if that's the case.
>> + * @base must be reachable from @bs, or NULL.
>>    */
>>   bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState 
>> *base,
>>                                     Error **errp)
>>   {
>>       BlockDriverState *i;
>> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
>> -        if (i->backing->frozen) {
>> +    for (i = bs; i != base; i = backing_bs(i)) {
>> +        if (i->backing && i->backing->frozen) {
>>               error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
>>                          i->backing->name, i->node_name,
>>                          backing_bs(i)->node_name);
>> @@ -4240,6 +4241,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState 
>> *bs, BlockDriverState *base,
>>    * Freeze all backing links between @bs and @base.
>>    * If any of the links is already frozen the operation is aborted and
>>    * none of the links are modified.
>> + * @base must be reachable from @bs, or NULL.
>>    * Returns 0 on success. On failure returns < 0 and sets @errp.
>>    */
>>   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
>> @@ -4251,8 +4253,10 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
>> BlockDriverState *base,
>>           return -EPERM;
>>       }
>> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
>> -        i->backing->frozen = true;
>> +    for (i = bs; i != base; i = backing_bs(i)) {
>> +        if (i->backing) {
>> +            i->backing->frozen = true;
>> +        }
>>       }
>>       return 0;
>> @@ -4261,14 +4265,17 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
>> BlockDriverState *base,
>>   /*
>>    * Unfreeze all backing links between @bs and @base. The caller must
>>    * ensure that all links are frozen before using this function.
>> + * @base must be reachable from @bs, or NULL.
>>    */
>>   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
>> *base)
>>   {
>>       BlockDriverState *i;
>> -    for (i = bs; i != base && i->backing; i = backing_bs(i)) {
>> -        assert(i->backing->frozen);
>> -        i->backing->frozen = false;
>> +    for (i = bs; i != base; i = backing_bs(i)) {
>> +        if (i->backing) {
>> +            assert(i->backing->frozen);
>> +            i->backing->frozen = false;
>> +        }
>>       }
>>   }
>>
> 
> 


-- 
Best regards,
Vladimir