On permission update commit we must set same permissions as on _check_.
Let's add assertions. Next step may be to drop permission parameters
from _set_.

Note that prior to previous commit, fixing bdrv_drop_intermediate(),
new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.

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

diff --git a/block.c b/block.c
index bd9f4e534b..0f4da59a6c 100644
--- a/block.c
+++ b/block.c
@@ -2105,9 +2105,10 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
     }
 }
 
-static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
-                          uint64_t cumulative_shared_perms)
+static void bdrv_set_perm(BlockDriverState *bs, uint64_t _cumulative_perms,
+                          uint64_t _cumulative_shared_perms)
 {
+    uint64_t cumulative_perms, cumulative_shared_perms;
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
 
@@ -2115,6 +2116,10 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
         return;
     }
 
+    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
+    assert(_cumulative_perms == cumulative_perms);
+    assert(_cumulative_shared_perms == cumulative_shared_perms);
+
     /* Update this node */
     if (drv->bdrv_set_perm) {
         drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
@@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
 
     c->has_backup_perm = false;
 
+    assert(c->perm == perm);
+    assert(c->shared_perm == shared);
     c->perm = perm;
     c->shared_perm = shared;
 
-- 
2.21.3

31.10.2020 15:35, Vladimir Sementsov-Ogievskiy wrote:
> On permission update commit we must set same permissions as on _check_.
> Let's add assertions. Next step may be to drop permission parameters
> from _set_.
> 
> Note that prior to previous commit, fixing bdrv_drop_intermediate(),
> new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bd9f4e534b..0f4da59a6c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2105,9 +2105,10 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
>       }
>   }
>   
> -static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
> -                          uint64_t cumulative_shared_perms)
> +static void bdrv_set_perm(BlockDriverState *bs, uint64_t _cumulative_perms,
> +                          uint64_t _cumulative_shared_perms)
>   {
> +    uint64_t cumulative_perms, cumulative_shared_perms;
>       BlockDriver *drv = bs->drv;
>       BdrvChild *c;
>   
> @@ -2115,6 +2116,10 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
>           return;
>       }
>   
> +    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
> +    assert(_cumulative_perms == cumulative_perms);
> +    assert(_cumulative_shared_perms == cumulative_shared_perms);
> +
>       /* Update this node */
>       if (drv->bdrv_set_perm) {
>           drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
> @@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
>   
>       c->has_backup_perm = false;
>   
> +    assert(c->perm == perm);
> +    assert(c->shared_perm == shared);
>       c->perm = perm;
>       c->shared_perm = shared;
>   
> 

squash:

@@ -2308,8 +2308,6 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
  
      assert(c->perm == perm);
      assert(c->shared_perm == shared);
-    c->perm = perm;
-    c->shared_perm = shared;
  
      bdrv_get_cumulative_perm(c->bs, &cumulative_perms,
                               &cumulative_shared_perms);



-- 
Best regards,
Vladimir

On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote:
> On permission update commit we must set same permissions as on _check_.
> Let's add assertions. Next step may be to drop permission parameters
> from _set_.
> 
> Note that prior to previous commit, fixing bdrv_drop_intermediate(),
> new assertion in bdrv_child_set_perm() crashes on iotests 30 and 40.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bd9f4e534b..0f4da59a6c 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -2301,6 +2306,8 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
>   
>       c->has_backup_perm = false;
>   
> +    assert(c->perm == perm);
> +    assert(c->shared_perm == shared);
>       c->perm = perm;
>       c->shared_perm = shared;

Then we can drop the assignments, no?

(And, as you write, in the future potentially drop the parameters.)

Anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>