Re: [PATCH v6 07/15] block: bdrv_check_perm(): process children anyway

[Vladimir Sementsov-Ogievskiy]

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


--
Best regards,
Vladimir