diff mbox

[for-2.10,4/4] block: Update open_flags after ->inactivate() callback

Message ID 20170823134242.12080-5-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Aug. 23, 2017, 1:42 p.m. UTC 
From: Stefan Hajnoczi <stefanha@redhat.com>

In the ->inactivate() callbacks, permissions are updated, which
typically involves a recursive check of the whole graph. Setting
BDRV_O_INACTIVE right before doing that creates a state that
bdrv_is_writable() returns false, which causes permission update
failure.

Reorder them so the flag is updated after calling the function. Note
that this doesn't break the assert in bdrv_child_cb_inactivate() because
for any specific BDS, we still update its flags first before calling
->inactivate() on it one level deeper in the recursion.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Stefan Hajnoczi Aug. 23, 2017, 2:11 p.m. UTC | #1 
On Wed, Aug 23, 2017 at 09:42:42PM +0800, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> In the ->inactivate() callbacks, permissions are updated, which
> typically involves a recursive check of the whole graph. Setting
> BDRV_O_INACTIVE right before doing that creates a state that
> bdrv_is_writable() returns false, which causes permission update
> failure.
> 
> Reorder them so the flag is updated after calling the function. Note
> that this doesn't break the assert in bdrv_child_cb_inactivate() because
> for any specific BDS, we still update its flags first before calling
> ->inactivate() on it one level deeper in the recursion.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox

Patch

diff --git a/block.c b/block.c
index 3615a6809e..3308814bba 100644
--- a/block.c
+++ b/block.c
@@ -4085,21 +4085,20 @@  static int bdrv_inactivate_recurse(BlockDriverState *bs,
         }
     }
 
-    if (setting_flag) {
+    if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
         uint64_t perm, shared_perm;
 
-        bs->open_flags |= BDRV_O_INACTIVE;
-
         QLIST_FOREACH(parent, &bs->parents, next_parent) {
             if (parent->role->inactivate) {
                 ret = parent->role->inactivate(parent);
                 if (ret < 0) {
-                    bs->open_flags &= ~BDRV_O_INACTIVE;
                     return ret;
                 }
             }
         }
 
+        bs->open_flags |= BDRV_O_INACTIVE;
+
         /* Update permissions, they may differ for inactive nodes */
         bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
         bdrv_check_perm(bs, perm, shared_perm, NULL, &error_abort);