[5/7] block: Don't queue the same BDS twice in bdrv_reopen_queue_child()

Commit Message

Alberto Garcia Sept. 14, 2016, 3:52 p.m. UTC

bdrv_reopen_queue_child() assumes that a BlockDriverState is never
added twice to BlockReopenQueue.

That's however not the case: commit_start() adds 'base' (and its
children) to a new reopen queue, and then 'overlay_bs' (and its
children, which include 'base') to the same queue. The effect of this
is that the first set of options is ignored and overriden by the
second.

We fixed this by swapping the order in which both BDSs were added to
the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch
checks if a BDS is already in the reopen queue and keeps its options.

Signed-off-by: Alberto Garcia <[e-mail]>
---
 block.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Kevin Wolf Sept. 15, 2016, 1:04 p.m. UTC | #1

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> bdrv_reopen_queue_child() assumes that a BlockDriverState is never
> added twice to BlockReopenQueue.
> 
> That's however not the case: commit_start() adds 'base' (and its
> children) to a new reopen queue, and then 'overlay_bs' (and its
> children, which include 'base') to the same queue. The effect of this
> is that the first set of options is ignored and overriden by the
> second.
> 
> We fixed this by swapping the order in which both BDSs were added to
> the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch
> checks if a BDS is already in the reopen queue and keeps its options.
> 
> Signed-off-by: Alberto Garcia <[e-mail]>

Reviewed-by: Kevin Wolf <[e-mail]>

Patch

diff --git a/block.c b/block.c
index cf9c513..57f4b2c 100644
--- a/block.c
+++ b/block.c
@@ -1874,6 +1874,13 @@  static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         options = qdict_new();
     }
 
+    /* Check if this BlockDriverState is already in the queue */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (bs == bs_entry->state.bs) {
+            break;
+        }
+    }
+
     /*
      * Precedence of options:
      * 1. Explicitly passed in options (highest)
@@ -1894,7 +1901,11 @@  static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     }
 
     /* Old explicitly set values (don't overwrite by inherited value) */
-    old_options = qdict_clone_shallow(bs->explicit_options);
+    if (bs_entry) {
+        old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
+    } else {
+        old_options = qdict_clone_shallow(bs->explicit_options);
+    }
     bdrv_join_options(bs, options, old_options);
     QDECREF(old_options);
 
@@ -1933,8 +1944,13 @@  static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
                                 child->role, options, flags);
     }
 
-    bs_entry = g_new0(BlockReopenQueueEntry, 1);
-    QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+    if (!bs_entry) {
+        bs_entry = g_new0(BlockReopenQueueEntry, 1);
+        QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+    } else {
+        QDECREF(bs_entry->state.options);
+        QDECREF(bs_entry->state.explicit_options);
+    }
 
     bs_entry->state.bs = bs;
     bs_entry->state.options = options;