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 <mreitz@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;
+ }
+
/* 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))
@@ -1798,11 +1820,16 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
/* Check all children */
QLIST_FOREACH(c, &bs->children, next) {
uint64_t cur_perm, cur_shared;
+ bool child_loosen_restr;
+
bdrv_child_perm(bs, c->bs, c, c->role, q,
cumulative_perms, cumulative_shared_perms,
&cur_perm, &cur_shared);
ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
- ignore_children, errp);
+ ignore_children, &child_loosen_restr, errp);
+ if (loosen_restrictions) {
+ *loosen_restrictions &= child_loosen_restr;
+ }
if (ret < 0) {
return ret;
}
@@ -1926,12 +1953,20 @@ char *bdrv_perm_names(uint64_t perm)
* set, the BdrvChild objects in this list are ignored in the calculations;
* this allows checking permission updates for an existing reference.
*
+ * 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.
+ *
* Needs to be followed by a call to either bdrv_set_perm() or
* bdrv_abort_perm_update(). */
static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
uint64_t new_used_perm,
uint64_t new_shared_perm,
- GSList *ignore_children, Error **errp)
+ GSList *ignore_children,
+ bool *loosen_restrictions,
+ Error **errp)
{
BdrvChild *c;
uint64_t cumulative_perms = new_used_perm;
@@ -1948,6 +1983,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
if ((new_used_perm & c->shared_perm) != new_used_perm) {
char *user = bdrv_child_user_desc(c);
char *perm_names = bdrv_perm_names(new_used_perm & ~c->shared_perm);
+
+ if (loosen_restrictions) {
+ *loosen_restrictions = false;
+ }
+
error_setg(errp, "Conflicts with use by %s as '%s', which does not "
"allow '%s' on %s",
user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1959,6 +1999,11 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
if ((c->perm & new_shared_perm) != c->perm) {
char *user = bdrv_child_user_desc(c);
char *perm_names = bdrv_perm_names(c->perm & ~new_shared_perm);
+
+ if (loosen_restrictions) {
+ *loosen_restrictions = false;
+ }
+
error_setg(errp, "Conflicts with use by %s as '%s', which uses "
"'%s' on %s",
user, c->name, perm_names, bdrv_get_node_name(c->bs));
@@ -1972,19 +2017,21 @@ static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
}
return bdrv_check_perm(bs, q, cumulative_perms, cumulative_shared_perms,
- ignore_children, errp);
+ ignore_children, loosen_restrictions, errp);
}
/* Needs to be followed by a call to either bdrv_child_set_perm() or
* bdrv_child_abort_perm_update(). */
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)
{
int ret;
ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
- ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
+ ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children,
+ loosen_restrictions, errp);
g_slist_free(ignore_children);
if (ret < 0) {
@@ -2037,7 +2084,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
{
int ret;
- ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, errp);
+ ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp);
if (ret < 0) {
bdrv_child_abort_perm_update(c);
return ret;
@@ -2223,7 +2270,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
* 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, NULL, perm, shared_perm, NULL, &error_abort);
+ bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
+ NULL, &error_abort);
bdrv_set_perm(old_bs, perm, shared_perm);
}
}
@@ -2237,7 +2285,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
BdrvChild *child;
int ret;
- ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
+ ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, NULL,
+ errp);
if (ret < 0) {
bdrv_abort_perm_update(child_bs);
return NULL;
@@ -3292,7 +3341,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
BDRVReopenState *state = &bs_entry->state;
ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
- state->shared_perm, NULL, errp);
+ state->shared_perm, NULL, NULL, errp);
if (ret < 0) {
goto cleanup_perm;
}
@@ -3304,7 +3353,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
state->perm, state->shared_perm,
&nperm, &nshared);
ret = bdrv_check_update_perm(state->new_backing_bs, NULL,
- nperm, nshared, NULL, errp);
+ nperm, nshared, NULL, NULL, errp);
if (ret < 0) {
goto cleanup_perm;
}
@@ -4031,7 +4080,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
/* Check whether the required permissions can be granted on @to, ignoring
* all BdrvChild in @list so that they can't block themselves. */
- ret = bdrv_check_update_perm(to, NULL, perm, shared, list, errp);
+ ret = bdrv_check_update_perm(to, NULL, perm, shared, list, NULL, errp);
if (ret < 0) {
bdrv_abort_perm_update(to);
goto out;
@@ -4378,7 +4427,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
/* Check whether we are allowed to switch c from top to base */
GSList *ignore_children = g_slist_prepend(NULL, c);
ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
- ignore_children, &local_err);
+ ignore_children, NULL, &local_err);
g_slist_free(ignore_children);
if (ret < 0) {
error_report_err(local_err);
@@ -5153,7 +5202,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
*/
bs->open_flags &= ~BDRV_O_INACTIVE;
bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
- ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+ ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &local_err);
if (ret < 0) {
bs->open_flags |= BDRV_O_INACTIVE;
error_propagate(errp, local_err);
@@ -5303,7 +5352,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
/* Update permissions, they may differ for inactive nodes */
bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
- bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
+ bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort);
bdrv_set_perm(bs, perm, shared_perm);
--
2.20.1
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 <mreitz@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
On 08.05.19 16:28, Kevin Wolf wrote:
> 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 <mreitz@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)
Naming things is hard. :-)
Should I name it tighten_restrictions and invert its value?
>> /* 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?
Well, I had that at one point, yes. I decided this case would always
imply that the restrictions are tightened somewhere, so I dropped it --
but I forgot about reopen, for some reason (even though it says “reopen”
right there).
> 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?
And add a note that we cannot return this information when reopening?
That sounds good to me.
Max
© 2016 - 2026 Red Hat, Inc.