qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 6/7] block: Add *loosen_restrictions to *check*_


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 6/7] block: Add *loosen_restrictions to *check*_perm()
Date: Wed, 8 May 2019 16:32:10 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

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 <address@hidden>
>> ---
>>  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, &current_perms, &current_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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]