Re: [PATCH v2 11/36] block: bdrv_refresh_perms: check parents compliance

[Vladimir Sementsov-Ogievskiy]

19.01.2021 20:42, Kevin Wolf wrote:
> Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Add additional check that node parents do not interfere with each
> > other. This should not hurt existing callers and allows in further
> > patch use bdrv_refresh_perms() to update a subtree of changed
> > BdrvChild (check that change is correct).
> >
> > New check will substitute bdrv_check_update_perm() in following
> > permissions refactoring, so keep error messages the same to avoid
> > unit test result changes.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> The change itself looks ok, but I'm not happy with the naming. It feels
> a bit unspecific. How about inverting the result and calling it
> bdrv_parent_perms_conflict() and bdrv_child_perms_conflict()?
>
> At least, I'd call it "permission consistency" rather then "compliance".

bdrv_parent_perms_conflict() sound good for me, OK

> > diff --git a/block.c b/block.c
> > index 29082c6d47..a756f3e8ad 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1966,6 +1966,57 @@ bool bdrv_is_writable(BlockDriverState *bs)
> >       return bdrv_is_writable_after_reopen(bs, NULL);
> >   }
> >   
> > +static char *bdrv_child_user_desc(BdrvChild *c)
> > +{
> > +    if (c->klass->get_parent_desc) {
> > +        return c->klass->get_parent_desc(c);
> > +    }
> > +
> > +    return g_strdup("another user");
> > +}
> > +
> > +static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
> > +{
> > +    g_autofree char *user = NULL;
> > +    g_autofree char *perm_names = NULL;
> > +
> > +    if ((b->perm & a->shared_perm) == b->perm) {
> > +        return true;
> > +    }
> > +
> > +    perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
> > +    user = bdrv_child_user_desc(a);
> > +    error_setg(errp, "Conflicts with use by %s as '%s', which does not "
> > +               "allow '%s' on %s",
> > +               user, a->name, perm_names, bdrv_get_node_name(b->bs));
> > +
> > +    return false;
> > +}
> > +
> > +static bool bdrv_check_parents_compliance(BlockDriverState *bs, Error **errp)
> > +{
> > +    BdrvChild *a, *b;
> > +
> > +    /*
> > +     * During the loop we'll look at each pair twice. That's correct is
>
> s/is/because/ or what did you mean here?

yes, s/is/because/

> > +     * bdrv_a_allow_b() is asymmetric and we should check each pair in both
> > +     * directions.
> > +     */
> > +    QLIST_FOREACH(a, &bs->parents, next_parent) {
> > +        QLIST_FOREACH(b, &bs->parents, next_parent) {
> > +            if (a == b) {
> > +                continue;
> > +            }
> > +
> > +            if (!bdrv_a_allow_b(a, b, errp)) {
> > +                return false;
> > +            }
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
>
> Kevin


--
Best regards,
Vladimir