Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> This patch makes three functions report whether the necessary permission
> change purely loosens restrictions or not. These functions are:
> - bdrv_check_perm()
> - bdrv_check_update_perm()
> - bdrv_child_check_perm()
>
> Callers can use this result to decide whether a failure is fatal or not
> (see the next patch).
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> block.c | 81 +++++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index 21e4514426..105866d15a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1686,9 +1686,12 @@ static int bdrv_fill_options(QDict **options, const
> char *filename,
>
> static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
> uint64_t perm, uint64_t shared,
> - GSList *ignore_children, Error **errp);
> + GSList *ignore_children,
> + bool *loosen_restrictions, Error **errp);
> static void bdrv_child_abort_perm_update(BdrvChild *c);
> static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t
> shared);
> +static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
> + uint64_t *shared_perm);
>
> typedef struct BlockReopenQueueEntry {
> bool prepared;
> @@ -1759,18 +1762,37 @@ static void bdrv_child_perm(BlockDriverState *bs,
> BlockDriverState *child_bs,
> * permissions of all its parents. This involves checking whether all
> necessary
> * permission changes to child nodes can be performed.
> *
> + * Will set *loosen_restrictions to true if and only if no new permissions
> have
> + * to be taken and no existing shared permissions are to be unshared. In
> this
> + * case, errors are not fatal, as long as the caller accepts that the
> + * restrictions remain tighter than they need to be. The caller still has to
> + * abort the transaction.
> + *
> * A call to this function must always be followed by a call to
> bdrv_set_perm()
> * or bdrv_abort_perm_update().
> */
> static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
> uint64_t cumulative_perms,
> uint64_t cumulative_shared_perms,
> - GSList *ignore_children, Error **errp)
> + GSList *ignore_children,
> + bool *loosen_restrictions, Error **errp)
> {
> BlockDriver *drv = bs->drv;
> BdrvChild *c;
> int ret;
>
> + if (loosen_restrictions) {
> + uint64_t current_perms, current_shared;
> + uint64_t added_perms, removed_shared_perms;
> +
> + bdrv_get_cumulative_perm(bs, ¤t_perms, ¤t_shared);
> +
> + added_perms = cumulative_perms & ~current_perms;
> + removed_shared_perms = current_shared & ~cumulative_shared_perms;
> +
> + *loosen_restrictions = !added_perms && !removed_shared_perms;
> + }
(loosen_restrictions is a misnomer, just not changing permissions will
make it true, too)
> /* Write permissions never work with read-only images */
> if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
> !bdrv_is_writable_after_reopen(bs, q))
{
error_setg(errp, "Block node is read-only");
return -EPERM;
}
This is an interesting case in the context of reopen. It could happen
that we're actually not taking any new permissions, but the node becomes
read-only in reopen, so we fail here while maintaining the old set of
options.
If this happens, we want the reopen operation to fail, so should we set
*loosen_restrictions = false here even though we're not literally taking
new permissions?
Hm, or actually, loosen_restrictions should always be NULL during
reopen, so it will never make a different. Maybe what we want then is
assert(!q || !loosen_restrictions) at the start of the function?
Kevin
