| Message ID | 20200918181951.21752-8-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | preallocate filter | expand |
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
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 >
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() */
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(-)