Re: [PATCH v2 36/36] block: refactor bdrv_node_check_perm() - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, armbru@redhat.com,
	jsnow@redhat.com, mreitz@redhat.com, den@openvz.org
Subject: Re: [PATCH v2 36/36] block: refactor bdrv_node_check_perm()
Date: Thu, 11 Feb 2021 12:50:36 +0300	[thread overview]
Message-ID: <73375283-eaea-2b9a-659e-22dd66569519@virtuozzo.com> (raw)
In-Reply-To: <20210210150708.GG5144@merkur.fritz.box>

10.02.2021 18:07, Kevin Wolf wrote:
> Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Now, bdrv_node_check_perm() is called only with fresh cumulative
>> permissions, so its actually "refresh_perm".
>>
>> Move permission calculation to the function. Also, drop unreachable
>> error message.
>>
>> Add also Virtuozzo copyright, as big work is done at this point.
> 
> I guess we could add many copyright lines then... Maybe we should, I
> don't know.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c | 38 +++++++++-----------------------------
>>   1 file changed, 9 insertions(+), 29 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 20b1cf59f7..576b145cbf 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2,6 +2,7 @@
>>    * QEMU System Emulator block driver
>>    *
>>    * Copyright (c) 2003 Fabrice Bellard
>> + * Copyright (c) 2020 Virtuozzo International GmbH.
>>    *
>>    * Permission is hereby granted, free of charge, to any person obtaining a copy
>>    * of this software and associated documentation files (the "Software"), to deal
>> @@ -2204,23 +2205,15 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
>>       /* old_bs reference is transparently moved from @child to @s */
>>   }
>>   
>> -/*
>> - * Check whether permissions on this node can be changed in a way that
>> - * @cumulative_perms and @cumulative_shared_perms are the new cumulative
>> - * permissions of all its parents. This involves checking whether all necessary
>> - * permission changes to child nodes can be performed.
>> - *
>> - * A call to this function must always be followed by a call to bdrv_set_perm()
>> - * or bdrv_abort_perm_update().
>> - */
> 
> Would you mind updating the comment rather than removing it?
> 
>> -static int bdrv_node_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>> -                                uint64_t cumulative_perms,
>> -                                uint64_t cumulative_shared_perms,
>> -                                GSList **tran, Error **errp)
>> +static int bdrv_node_refresh_perm(BlockDriverState *bs, BlockReopenQueue *q,
>> +                                  GSList **tran, Error **errp)
>>   {
>>       BlockDriver *drv = bs->drv;
>>       BdrvChild *c;
>>       int ret;
>> +    uint64_t cumulative_perms, cumulative_shared_perms;
>> +
>> +    bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
>>   
>>       /* Write permissions never work with read-only images */
>>       if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
>> @@ -2229,15 +2222,8 @@ static int bdrv_node_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
>>           if (!bdrv_is_writable_after_reopen(bs, NULL)) {
>>               error_setg(errp, "Block node is read-only");
>>           } else {
>> -            uint64_t current_perms, current_shared;
>> -            bdrv_get_cumulative_perm(bs, &current_perms, &current_shared);
>> -            if (current_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) {
>> -                error_setg(errp, "Cannot make block node read-only, there is "
>> -                           "a writer on it");
>> -            } else {
>> -                error_setg(errp, "Cannot make block node read-only and create "
>> -                           "a writer on it");
>> -            }
>> +            error_setg(errp, "Cannot make block node read-only, there is "
>> +                       "a writer on it");
> 
> Hm, so if you want to add a new writer to an existing read-only node,
> this is the error message that you would get?
> 
> Now that we can't distinguish both cases any more, should we try to
> rephrase it so that it makes sense for both directions? Like "Read-only
> block node <node-name> cannot support read-write users"?
> 

OK.

> 
> Sorry for it taking so long, but I've now finally looked at all patches
> in this series. Please feel free to send v3 when you're ready.
> 
Thanks a lot for reviewing!

-- 
Best regards,
Vladimir