[v6] preallocate filter

[PATCH v6 07/15] block: bdrv_check_perm(): process children anyway

Posted by Vladimir Sementsov-Ogievskiy 5 years, 4 months ago

Do generic processing even for drivers which define .bdrv_check_perm
handler. It's needed for further preallocate filter: it will need to do
additional action on bdrv_check_perm, but don't want to reimplement
generic logic.

The patch doesn't change existing behaviour: the only driver that
implements bdrv_check_perm is file-posix, but it never has any
children.

Also, bdrv_set_perm() don't stop processing if driver has
.bdrv_set_perm handler as well.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 9538af4884..165c2d3cb2 100644
--- a/block.c
+++ b/block.c
@@ -1964,8 +1964,7 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
- * permissions of all its parents. This involves checking whether all necessary
- * permission changes to child nodes can be performed.
+ * permissions of all its parents.
  *
  * Will set *tighten_restrictions to true if and only if new permissions have to
  * be taken or currently shared permissions are to be unshared.  Otherwise,
@@ -2047,8 +2046,11 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
     }
 
     if (drv->bdrv_check_perm) {
-        return drv->bdrv_check_perm(bs, cumulative_perms,
-                                    cumulative_shared_perms, errp);
+        ret = drv->bdrv_check_perm(bs, cumulative_perms,
+                                   cumulative_shared_perms, errp);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     /* Drivers that never have children can omit .bdrv_child_perm() */
-- 
2.21.3

Re: [PATCH v6 07/15] block: bdrv_check_perm(): process children anyway

Posted by Max Reitz 5 years, 4 months ago

On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Do generic processing even for drivers which define .bdrv_check_perm
> handler. It's needed for further preallocate filter: it will need to do
> additional action on bdrv_check_perm, but don't want to reimplement
> generic logic.
> 
> The patch doesn't change existing behaviour: the only driver that
> implements bdrv_check_perm is file-posix, but it never has any
> children.
> 
> Also, bdrv_set_perm() don't stop processing if driver has
> .bdrv_set_perm handler as well.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9538af4884..165c2d3cb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1964,8 +1964,7 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
>  /*
>   * Check whether permissions on this node can be changed in a way that
>   * @cumulative_perms and @cumulative_shared_perms are the new cumulative
> - * permissions of all its parents. This involves checking whether all necessary
> - * permission changes to child nodes can be performed.
> + * permissions of all its parents.

Why do you want to remove this sentence?

>   *
>   * Will set *tighten_restrictions to true if and only if new permissions have to
>   * be taken or currently shared permissions are to be unshared.  Otherwise,
> @@ -2047,8 +2046,11 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>      }
>  
>      if (drv->bdrv_check_perm) {
> -        return drv->bdrv_check_perm(bs, cumulative_perms,
> -                                    cumulative_shared_perms, errp);
> +        ret = drv->bdrv_check_perm(bs, cumulative_perms,
> +                                   cumulative_shared_perms, errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }

Sounds good.  It’s also consistent with how bdrv_abort_perm_update() and
bdrv_set_perm() don’t return after calling the respective driver
functions, but always recurse to the children.

Max

Re: [PATCH v6 07/15] block: bdrv_check_perm(): process children anyway

Posted by Vladimir Sementsov-Ogievskiy 5 years, 4 months ago

24.09.2020 17:25, Max Reitz wrote:
> On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
>> Do generic processing even for drivers which define .bdrv_check_perm
>> handler. It's needed for further preallocate filter: it will need to do
>> additional action on bdrv_check_perm, but don't want to reimplement
>> generic logic.
>>
>> The patch doesn't change existing behaviour: the only driver that
>> implements bdrv_check_perm is file-posix, but it never has any
>> children.
>>
>> Also, bdrv_set_perm() don't stop processing if driver has
>> .bdrv_set_perm handler as well.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9538af4884..165c2d3cb2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1964,8 +1964,7 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
>>   /*
>>    * Check whether permissions on this node can be changed in a way that
>>    * @cumulative_perms and @cumulative_shared_perms are the new cumulative
>> - * permissions of all its parents. This involves checking whether all necessary
>> - * permission changes to child nodes can be performed.
>> + * permissions of all its parents.
> 
> Why do you want to remove this sentence?

Really strange :) I don't know. I remember that I've modified some comment working on this series, and it was important... But this sentence become even more obviously correct with this patch.

> 
>>    *
>>    * Will set *tighten_restrictions to true if and only if new permissions have to
>>    * be taken or currently shared permissions are to be unshared.  Otherwise,
>> @@ -2047,8 +2046,11 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>>       }
>>   
>>       if (drv->bdrv_check_perm) {
>> -        return drv->bdrv_check_perm(bs, cumulative_perms,
>> -                                    cumulative_shared_perms, errp);
>> +        ret = drv->bdrv_check_perm(bs, cumulative_perms,
>> +                                   cumulative_shared_perms, errp);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>       }
> 
> Sounds good.  It’s also consistent with how bdrv_abort_perm_update() and
> bdrv_set_perm() don’t return after calling the respective driver
> functions, but always recurse to the children.
> 
> Max
> 


-- 
Best regards,
Vladimir