Re: [Qemu-devel] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2335 bytes --]

On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> Use child access functions when iterating through backing chains so
>> filters do not break the chain.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 40 ++++++++++++++++++++++++++++------------
>>   1 file changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 11f37983d9..505b3e9a01 100644
>> --- a/block.c
>> +++ b/block.c

[...]

>> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>   BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>>                                       BlockDriverState *bs)
>>   {
>> -    while (active && bs != backing_bs(active)) {
>> -        active = backing_bs(active);
>> +    bs = bdrv_skip_rw_filters(bs);
>> +    active = bdrv_skip_rw_filters(active);
>> +
>> +    while (active) {
>> +        BlockDriverState *next = bdrv_backing_chain_next(active);
>> +        if (bs == next) {
>> +            return active;
>> +        }
>> +        active = next;
>>       }
>>   
>> -    return active;
>> +    return NULL;
>>   }
> 
> Semantics changed for this function.
> It is used in two places
> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we will never have
>     filter node as a bottom of some valid chain
> 
> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't understand,
> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs overlay is out of the job,
> what is this check for?

There is a loop before this check which checks that the same blocker is
not set on any nodes between top and base (both inclusive).  I guess
non-active commit checks the node above @top, too, because its backing
file will change.

>>   /* Given a BDS, searches for the base layer. */

[...]

>> @@ -5149,7 +5165,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>               char *backing_file_full_ret;
>>   
>>               if (strcmp(backing_file, curr_bs->backing_file) == 0) {
> 
> hmm, interesting, what bs->backing_file now means? It's strange enough to store such field on
> bds, when we have backing link anyway..

Patch 37 has you covered. :-)

Max


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