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.

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().
- */
-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");
         }
 
         return -EPERM;
@@ -2293,7 +2279,6 @@ static int bdrv_list_refresh_perms(GSList *list, BlockReopenQueue *q,
                                    GSList **tran, Error **errp)
 {
     int ret;
-    uint64_t cumulative_perms, cumulative_shared_perms;
     BlockDriverState *bs;
 
     for ( ; list; list = list->next) {
@@ -2303,12 +2288,7 @@ static int bdrv_list_refresh_perms(GSList *list, BlockReopenQueue *q,
             return -EINVAL;
         }
 
-        bdrv_get_cumulative_perm(bs, &cumulative_perms,
-                                 &cumulative_shared_perms);
-
-        ret = bdrv_node_check_perm(bs, q, cumulative_perms,
-                                   cumulative_shared_perms,
-                                   tran, errp);
+        ret = bdrv_node_refresh_perm(bs, q, tran, errp);
         if (ret < 0) {
             return ret;
         }
-- 
2.21.3

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"?


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.

Kevin

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