1. Patchew
  2. QEMU
  3. [PATCH v2 0/7] block: permission update fix & refactor
  4. View patch

[PATCH v2 4/7] block: add bdrv_refresh_perms() helper

Vladimir Sementsov-Ogievskiy posted 7 patches 5 years, 3 months ago
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
[PATCH v2 4/7] block: add bdrv_refresh_perms() helper
Posted by Vladimir Sementsov-Ogievskiy 5 years, 3 months ago 
Make separate function for common pattern.

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

diff --git a/block.c b/block.c
index 77a3f8f1e2..fc7633307f 100644
--- a/block.c
+++ b/block.c
@@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
     bdrv_abort_perm_update(c->bs);
 }
 
+static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
+                              Error **errp)
+{
+    int ret;
+    uint64_t perm, shared_perm;
+
+    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+    if (ret < 0) {
+        bdrv_abort_perm_update(bs);
+        return ret;
+    }
+    bdrv_set_perm(bs, perm, shared_perm);
+
+    return 0;
+}
+
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp)
 {
@@ -2636,22 +2653,15 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
     }
 
     if (old_bs) {
-        /* Update permissions for old node. This is guaranteed to succeed
-         * because we're just taking a parent away, so we're loosening
-         * restrictions. */
         bool tighten_restrictions;
-        int ret;
 
-        bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
-                              &tighten_restrictions, NULL);
+        /*
+         * Update permissions for old node. We're just taking a parent away, so
+         * we're loosening restrictions. Errors of permission update are not
+         * fatal in this case, ignore them.
+         */
+        bdrv_refresh_perms(old_bs, &tighten_restrictions, NULL);
         assert(tighten_restrictions == false);
-        if (ret < 0) {
-            /* We only tried to loosen restrictions, so errors are not fatal */
-            bdrv_abort_perm_update(old_bs);
-        } else {
-            bdrv_set_perm(old_bs, perm, shared_perm);
-        }
 
         /* When the parent requiring a non-default AioContext is removed, the
          * node moves back to the main AioContext */
@@ -5760,7 +5770,6 @@ void bdrv_init_with_whitelist(void)
 int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BdrvChild *child, *parent;
-    uint64_t perm, shared_perm;
     Error *local_err = NULL;
     int ret;
     BdrvDirtyBitmap *bm;
@@ -5792,14 +5801,11 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
      */
     if (bs->open_flags & BDRV_O_INACTIVE) {
         bs->open_flags &= ~BDRV_O_INACTIVE;
-        bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-        ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+        ret = bdrv_refresh_perms(bs, NULL, errp);
         if (ret < 0) {
-            bdrv_abort_perm_update(bs);
             bs->open_flags |= BDRV_O_INACTIVE;
             return ret;
         }
-        bdrv_set_perm(bs, perm, shared_perm);
 
         if (bs->drv->bdrv_co_invalidate_cache) {
             bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
@@ -5875,7 +5881,6 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
     BdrvChild *child, *parent;
     bool tighten_restrictions;
-    uint64_t perm, shared_perm;
     int ret;
 
     if (!bs->drv) {
@@ -5909,18 +5914,13 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 
     bs->open_flags |= BDRV_O_INACTIVE;
 
-    /* Update permissions, they may differ for inactive nodes */
-    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
-                          &tighten_restrictions, NULL);
+    /*
+     * Update permissions, they may differ for inactive nodes.
+     * We only tried to loosen restrictions, so errors are not fatal, ignore
+     * them.
+     */
+    bdrv_refresh_perms(bs, &tighten_restrictions, NULL);
     assert(tighten_restrictions == false);
-    if (ret < 0) {
-        /* We only tried to loosen restrictions, so errors are not fatal */
-        bdrv_abort_perm_update(bs);
-    } else {
-        bdrv_set_perm(bs, perm, shared_perm);
-    }
-
 
     /* Recursively inactivate children */
     QLIST_FOREACH(child, &bs->children, next) {
-- 
2.21.3
Re: [PATCH v2 4/7] block: add bdrv_refresh_perms() helper
Posted by Alberto Garcia 5 years, 3 months ago 
On Fri 06 Nov 2020 01:42:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Make separate function for common pattern.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 60 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/block.c b/block.c
> index 77a3f8f1e2..fc7633307f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
>      bdrv_abort_perm_update(c->bs);
>  }
>  
> +static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
> +                              Error **errp)
> +{
> +    int ret;
> +    uint64_t perm, shared_perm;
> +
> +    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
> +    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
> errp);

Aren't you supposed to pass tighten_restrictions here ?

Berto
Re: [PATCH v2 4/7] block: add bdrv_refresh_perms() helper
Posted by Vladimir Sementsov-Ogievskiy 5 years, 2 months ago 
06.11.2020 18:14, Alberto Garcia wrote:
> On Fri 06 Nov 2020 01:42:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> Make separate function for common pattern.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c | 60 ++++++++++++++++++++++++++++-----------------------------
>>   1 file changed, 30 insertions(+), 30 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 77a3f8f1e2..fc7633307f 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
>>       bdrv_abort_perm_update(c->bs);
>>   }
>>   
>> +static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
>> +                              Error **errp)
>> +{
>> +    int ret;
>> +    uint64_t perm, shared_perm;
>> +
>> +    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
>> +    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
>> errp);
> 
> Aren't you supposed to pass tighten_restrictions here ?
> 

Oops, yes I should


-- 
Best regards,
Vladimir
Re: [PATCH v2 4/7] block: add bdrv_refresh_perms() helper
Posted by Vladimir Sementsov-Ogievskiy 5 years, 2 months ago 
09.11.2020 10:04, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2020 18:14, Alberto Garcia wrote:
>> On Fri 06 Nov 2020 01:42:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>> Make separate function for common pattern.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c | 60 ++++++++++++++++++++++++++++-----------------------------
>>>   1 file changed, 30 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 77a3f8f1e2..fc7633307f 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
>>>       bdrv_abort_perm_update(c->bs);
>>>   }
>>> +static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
>>> +                              Error **errp)
>>> +{
>>> +    int ret;
>>> +    uint64_t perm, shared_perm;
>>> +
>>> +    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
>>> +    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
>>> errp);
>>
>> Aren't you supposed to pass tighten_restrictions here ?
>>
> 
> Oops, yes I should
> 

So, squash-in:


diff --git a/block.c b/block.c
index fc7633307f..a96dc07364 100644
--- a/block.c
+++ b/block.c
@@ -2328,7 +2328,8 @@ static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
      uint64_t perm, shared_perm;
  
      bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
+                          tighten_restrictions, errp);
      if (ret < 0) {
          bdrv_abort_perm_update(bs);
          return ret;



(produces simple conflict when applying "block: drop tighten_restrictions")

-- 
Best regards,
Vladimir

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.