diff mbox series

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

Message ID 20201106124241.16950-5-vsementsov@virtuozzo.com
State Accepted
Commit bb87e4d1c0c34d55c6fdecaf20b6f9640498e1ad
Headers show
Series block: permission update fix & refactor | expand

Commit Message

Vladimir Sementsov-Ogievskiy Nov. 6, 2020, 12:42 p.m. UTC 
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(-)

Comments

Alberto Garcia Nov. 6, 2020, 3:14 p.m. UTC | #1 
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
Vladimir Sementsov-Ogievskiy Nov. 9, 2020, 7:04 a.m. UTC | #2 
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
Vladimir Sementsov-Ogievskiy Nov. 9, 2020, 1:44 p.m. UTC | #3 
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
diff mbox series

Patch

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) {