| Message ID | 20201127144522.29991-12-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
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". > 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? > + * 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
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 >
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 + * 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; +} + static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs, BdrvChild *c, BdrvChildRole role, BlockReopenQueue *reopen_queue, @@ -2143,15 +2194,6 @@ void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, *shared_perm = cumulative_shared_perms; } -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"); -} - char *bdrv_perm_names(uint64_t perm) { struct perm_name { @@ -2295,6 +2337,9 @@ static int bdrv_refresh_perms(BlockDriverState *bs, Error **errp) int ret; uint64_t perm, shared_perm; + if (!bdrv_check_parents_compliance(bs, errp)) { + return -EPERM; + } bdrv_get_cumulative_perm(bs, &perm, &shared_perm); ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, errp); if (ret < 0) {
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> --- block.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 9 deletions(-)