Re: [PATCH 04/21] block: bdrv_refresh_perms: check parents compliance - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, armbru@redhat.com, jsnow@redhat.com,
	mreitz@redhat.com, kwolf@redhat.com, den@openvz.org
Subject: Re: [PATCH 04/21] block: bdrv_refresh_perms: check parents compliance
Date: Tue, 24 Nov 2020 12:42:46 +0300	[thread overview]
Message-ID: <97ec2117-470e-aac6-7861-903c97e91b97@virtuozzo.com> (raw)
In-Reply-To: <20201123201233.9534-7-vsementsov@virtuozzo.com>

23.11.2020 23:12, Vladimir Sementsov-Ogievskiy wrote:
> 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 | 62 ++++++++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0dd28f0902..0d0f065db4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1945,6 +1945,56 @@ 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;
> +
> +    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;
> +            }
> +
> +            if (!bdrv_a_allow_b(b, a, errp)) {
> +                return false;
> +            }

drop this if. We look at each pair twice anyway.

-- 
Best regards,
Vladimir