Re: [PATCH v7 01/47] block: Add child access functions

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

28.07.2020 19:09, Christophe de Dinechin wrote:
> 
> On 2020-07-13 at 11:06 CEST, Vladimir Sementsov-Ogievskiy wrote...
>> 25.06.2020 18:21, Max Reitz wrote:
>>> There are BDS children that the general block layer code can access,
>>> namely bs->file and bs->backing.  Since the introduction of filters and
>>> external data files, their meaning is not quite clear.  bs->backing can
>>> be a COW source, or it can be a filtered child; bs->file can be a
>>> filtered child, it can be data and metadata storage, or it can be just
>>> metadata storage.
>>>
>>> This overloading really is not helpful.  This patch adds functions that
>>> retrieve the correct child for each exact purpose.  Later patches in
>>> this series will make use of them.  Doing so will allow us to handle
>>> filter nodes in a meaningful way.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>
>> [..]
>>
>>> +/*
>>> + * Return the primary child of this node: For filters, that is the
>>> + * filtered child.  For other nodes, that is usually the child storing
>>> + * metadata.
>>> + * (A generally more helpful description is that this is (usually) the
>>> + * child that has the same filename as @bs.)
>>> + *
>>> + * Drivers do not necessarily have a primary child; for example quorum
>>> + * does not.
>>> + */
>>> +BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>> +{
>>> +    BdrvChild *c;
>>> +
>>> +    QLIST_FOREACH(c, &bs->children, next) {
>>> +        if (c->role & BDRV_CHILD_PRIMARY) {
>>> +            return c;
>>> +        }
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>>
>>
>> Suggest squash-in to also assert that not more than one primary child:
>> --- a/block.c
>> +++ b/block.c
>> @@ -6998,13 +6998,14 @@ BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs)
>>     */
>>    BdrvChild *bdrv_primary_child(BlockDriverState *bs)
>>    {
>> -    BdrvChild *c;
>> +    BdrvChild *c, *found = NULL;
>>
>>        QLIST_FOREACH(c, &bs->children, next) {
>>            if (c->role & BDRV_CHILD_PRIMARY) {
>> -            return c;
>> +            assert(!found);
>> +            found = c;
>>            }
>>        }
>>
>> -    return NULL;
>> +    return c;
> 
> Shouldn't that be "return found"?

Oops, you are right of course!

>>    }
>>
>>
>> with or without:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> 


-- 
Best regards,
Vladimir