diff mbox

[for-2.10,1/5] block: Fix order in bdrv_replace_child()

Message ID 20170803150301.10177-2-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Wolf Aug. 3, 2017, 3:02 p.m. UTC
Commit 8ee03995 refactored the code incorrectly and broke the release of
permissions on the old BDS. Instead of changing the permissions to the
new required values after removing the old BDS from the list of
children, it only re-obtains the permissions it already had.

Change the order of operations so that the old BDS is removed again
before calculating the new required permissions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Blake Aug. 3, 2017, 3:12 p.m. UTC | #1
On 08/03/2017 10:02 AM, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>
Jeff Cody Aug. 3, 2017, 4:18 p.m. UTC | #2
On Thu, Aug 03, 2017 at 05:02:57PM +0200, Kevin Wolf wrote:
> Commit 8ee03995 refactored the code incorrectly and broke the release of
> permissions on the old BDS. Instead of changing the permissions to the
> new required values after removing the old BDS from the list of
> children, it only re-obtains the permissions it already had.
> 
> Change the order of operations so that the old BDS is removed again
> before calculating the new required permissions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7b3c..ab908cdc50 100644
> --- a/block.c
> +++ b/block.c
> @@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>      BlockDriverState *old_bs = child->bs;
>      uint64_t perm, shared_perm;
>  
> +    bdrv_replace_child_noperm(child, new_bs);
> +
>      if (old_bs) {
>          /* Update permissions for old node. This is guaranteed to succeed
>           * because we're just taking a parent away, so we're loosening
> @@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>          bdrv_set_perm(old_bs, perm, shared_perm);
>      }
>  
> -    bdrv_replace_child_noperm(child, new_bs);
> -
>      if (new_bs) {
>          bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
>          bdrv_set_perm(new_bs, perm, shared_perm);
> -- 
> 2.13.3
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>
diff mbox

Patch

diff --git a/block.c b/block.c
index ce9cce7b3c..ab908cdc50 100644
--- a/block.c
+++ b/block.c
@@ -1933,6 +1933,8 @@  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
     BlockDriverState *old_bs = child->bs;
     uint64_t perm, shared_perm;
 
+    bdrv_replace_child_noperm(child, new_bs);
+
     if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
@@ -1942,8 +1944,6 @@  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
-    bdrv_replace_child_noperm(child, new_bs);
-
     if (new_bs) {
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         bdrv_set_perm(new_bs, perm, shared_perm);