Re: [PATCH v5 04/45] test-bdrv-graph-mod: update test_parallel_perm_update test case

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

On 6/7/22 13:53, Hanna Reitz wrote:
> On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
>> test_parallel_perm_update() does two things that we are going to
>> restrict in the near future:
>>
>> 1. It updates bs->file field by hand. bs->file will be managed
>>     automatically by generic code (together with bs->children list).
>>
>>     Let's better refactor our "tricky" bds to have own state where one
>>     of children is linked as "selected".
>>     This also looks less "tricky", so avoid using this word.
>>
>> 2. It create FILTERED children that are not PRIMARY. Except for tests
>>     all FILTERED children in the Qemu block layer are always PRIMARY as
>>     well.  We are going to formalize this rule, so let's better use DATA
>>     children here.
> 
> Another thing is that any node may have at most one FILTERED child at a time, which was already formalized in BDRV_CHILD_FILTERED’s description.

Right, will add

> 
>> While being here, update the picture to better correspond to the test
>> code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
> 
> The change looks good, I’m just a bit confused when it comes to the comment describing what’s going on.
> 
>>   tests/unit/test-bdrv-graph-mod.c | 70 ++++++++++++++++++++------------
>>   1 file changed, 44 insertions(+), 26 deletions(-)
>>
>> diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
>> index a6e3bb79be..40795d3c04 100644
>> --- a/tests/unit/test-bdrv-graph-mod.c
>> +++ b/tests/unit/test-bdrv-graph-mod.c
> 
> [...]
> 
>> @@ -266,15 +280,18 @@ static BlockDriver bdrv_write_to_file = {
>>    * The following test shows that topological-sort order is required for
>>    * permission update, simple DFS is not enough.
>>    *
>> - * Consider the block driver which has two filter children: one active
>> - * with exclusive write access and one inactive with no specific
>> - * permissions.
>> + * Consider the block driver (write-to-selected) which has two children: one is
>> + * selected so we have exclusive write access to it and for the other one we
>> + * don't need any specific permissions.
>>    *
>>    * And, these two children has a common base child, like this:
>> + *   (additional "top" on top is used in test just because the only public
>> + *    function to update permission should get a specific child to update.
>> + *    Making bdrv_refresh_perms() public just for this test doesn't worth it)
> 
> s/doesn't/isn't/
> 
>>    *
>> - * ┌─────┐     ┌──────┐
>> - * │ fl2 │ ◀── │ top  │
>> - * └─────┘     └──────┘
>> + * ┌─────┐     ┌───────────────────┐     ┌─────┐
>> + * │ fl2 │ ◀── │ write-to-selected │ ◀── │ top │
>> + * └─────┘     └───────────────────┘     └─────┘
>>    *   │           │
>>    *   │           │ w
>>    *   │           ▼
>> @@ -290,7 +307,7 @@ static BlockDriver bdrv_write_to_file = {
>>    *
>>    * So, exclusive write is propagated.
>>    *
>> - * Assume, we want to make fl2 active instead of fl1.
>> + * Assume, we want to select fl2  instead of fl1.
> 
> There’s a double space after “fl2”.
> 
>>    * So, we set some option for top driver and do permission update.
> 
> Here and in the rest of the comment, it’s now unclear what node “top” refers to.  I think it’s still the now-renamed “write-to-selected” node, right?  But “top” is now a different node, so I’m not 100% sure.

Right, will fix.

> 
> (On the other hand, even before this patch, there was a “top” node that was distinct from the former “tricky” node...  So it seems like this comment was already not quite right before?)

Hmm yes. Obviously I tried to make this more obvious, but didn't update the whole comment.

> 
>>    *
>>    * With simple DFS, if permission update goes first through
> 
> 


-- 
Best regards,
Vladimir