Re: [PATCH 07/22] blkverify: Implement .bdrv_recurse_can_replace()

[Max Reitz <mreitz@redhat.com>]

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

On 25.09.19 14:56, Vladimir Sementsov-Ogievskiy wrote:
> 20.09.2019 18:27, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkverify.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 304b0a1368..0add3ab483 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -282,6 +282,20 @@ static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
>>       return bdrv_recurse_is_first_non_filter(s->test_file->bs, candidate);
>>   }
>>   
>> +static bool blkverify_recurse_can_replace(BlockDriverState *bs,
>> +                                          BlockDriverState *to_replace)
>> +{
>> +    BDRVBlkverifyState *s = bs->opaque;
>> +
>> +    /*
>> +     * blkverify quits the whole qemu process if there is a mismatch
>> +     * between bs->file->bs and s->test_file->bs.  Therefore, we know
>> +     * know that both must match bs and we can recurse down to either.
>> +     */
>> +    return bdrv_recurse_can_replace(bs->file->bs, to_replace) ||
>> +           bdrv_recurse_can_replace(s->test_file->bs, to_replace);
> 
> Ok, now I understand, what bdrv_recurse_can_replace actually does:
> 
> It searches for to_replace in bs-rooted subtree, going only through equal
> children..
> 
> So, we can replace @to_replace, by something equal to @bs, if @to_replace is
> in equal-subtree of @bs.
> 
> I'll try to explain my misleading:
> 
> bdrv_recurse_can_replace declaration looks like bs and to_replace may be absolutely
> unrelated nodes. So, why bs should decide, can we replace the unrelated to_replace
> node by something..

I thought the comment above bdrv_recurse_can_replace() made that clear:
“To be replaceable, @bs and @to_replace may either be guaranteed to
always show the same data (because they are only connected through
filters), or some driver may allow replacing one of its children because
it can guarantee that this child's data is not visible at all (for
example, for dissenting quorum children that have no other parents).”

> So, it may be simpler to follow, if it was called bdrv_recurse_filtered_subtree, or
> bdrv_recurse_transparent_subtree..
> 
> Still, now I understand, and don't care. It's better anyway than bdrv_recurse_is_first_non_filter
> 
>> +}
>> +
>>   static void blkverify_refresh_filename(BlockDriverState *bs)
>>   {
>>       BDRVBlkverifyState *s = bs->opaque;
>> @@ -328,6 +342,7 @@ static BlockDriver bdrv_blkverify = {
>>   
>>       .is_filter                        = true,
>>       .bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
>> +    .bdrv_recurse_can_replace         = blkverify_recurse_can_replace,
> 
> it will be never called, as bdrv_recurse_can_replace handles filters by itself.

(As I’ve just replied to the previous patch, I think
bdrv_recurse_can_replace() is wrong about that.)

Max


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