On 28.02.2017 13:53, Kevin Wolf wrote: > In many cases, the required permissions of one node on its children > depend on what its parents require from it. For example, the raw format > or most filter drivers only need to request consistent reads if that's > something that one of their parents wants. > > In order to achieve this, this patch introduces two new BlockDriver > callbacks. The first one lets drivers first check (recursively) whether > the requested permissions can be set; the second one actually sets the > new permission bitmask. > > Also add helper functions that drivers can use in their implementation > of the callbacks to update their permissions on a specific child. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > block.c | 206 > +++++++++++++++++++++++++++++++++++++++++++++- > include/block/block_int.h | 61 ++++++++++++++ > 2 files changed, 263 insertions(+), 4 deletions(-)
Reviewed-by: Max Reitz <mre...@redhat.com>
Some irrelevant nagging below.
> diff --git a/block.c b/block.c
> index 9628c7a..cf3534f 100644
> --- a/block.c
> +++ b/block.c
[...]
> -static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
> +static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
> + bool check_new_perm)
I can't say I like this parameter name very much, but I can't come up
with anything better.
> {
> BlockDriverState *old_bs = child->bs;
> + uint64_t perm, shared_perm;
>
> if (old_bs) {
> if (old_bs->quiesce_counter && child->role->drained_end) {
> child->role->drained_end(child);
> }
> QLIST_REMOVE(child, next_parent);
> +
> + /* Update permissions for old node. This is guaranteed to succeed
> + * because we're just taking a parent away, so we're loosening
> + * restrictions. */
> + bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
> + bdrv_check_perm(old_bs, perm, shared_perm, &error_abort);
> + bdrv_set_perm(old_bs, perm, shared_perm);
> }
>
> child->bs = new_bs;
> @@ -1376,6 +1564,12 @@ static void bdrv_replace_child(BdrvChild *child,
> BlockDriverState *new_bs)
> if (new_bs->quiesce_counter && child->role->drained_begin) {
> child->role->drained_begin(child);
> }
> +
> + bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
> + if (check_new_perm) {
> + bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);
Yeah, well, error_abort is not very good. But can be fixed later...
(We should at least pass it through and make that decision in
change_parent_backing_link().)
> + }
> + bdrv_set_perm(new_bs, perm, shared_perm);
> }
> }
>
> @@ -1390,6 +1584,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
> *child_bs,
>
> ret = bdrv_check_update_perm(child_bs, perm, shared_perm, NULL, errp);
> if (ret < 0) {
> + bdrv_abort_perm_update(child_bs);
> return NULL;
> }
>
> @@ -1403,7 +1598,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
> *child_bs,
> .opaque = opaque,
> };
>
> - bdrv_replace_child(child, child_bs);
> + /* This performs the matching bdrv_set_perm() for the above check. */
> + bdrv_replace_child(child, child_bs, false);
>
> return child;
> }
> @@ -1434,7 +1630,7 @@ static void bdrv_detach_child(BdrvChild *child)
> child->next.le_prev = NULL;
> }
>
> - bdrv_replace_child(child, NULL);
> + bdrv_replace_child(child, NULL, false);
The bool is basically ignored, but I'd rather set it to true here
because there is no check call before; so if a new child was attached
(which it clearly isn't), we would indeed want check_new_perm = true.
(You see, just irrelevant nagging.)
Max
>
> g_free(child->name);
> g_free(child);
signature.asc
Description: OpenPGP digital signature
