| Message ID | 20201031123502.4558-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix nested permission update | expand |
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);
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>
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;
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(-)