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 <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)
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
signature.asc
Description: OpenPGP digital signature
