Re: [Qemu-devel] [QEMU 2.0 Fix] block: make bdrv_swap rebuild the bs graph node list field.

[Benoît Canet]

The Wednesday 05 Mar 2014 à 23:22:30 (+0100), Max Reitz wrote :
> On 04.03.2014 17:42, Benoît Canet wrote:
> >Moving only the node_name one field could lead to some inconsitencies where a
> >node_name was defined on a bs which was not registered in the graph node 
> >list.
> >
> >bdrv_swap between a named node bs and a non named node bs would lead to this.
> >
> >bdrv_make_anon would then crash because it would try to remove the bs from 
> >the
> >graph node list while it is not in it.
> >
> >This patch remove named node bses from the graph node list before doing the 
> >swap
> >then insert them back.
> >
> >Signed-off-by: Benoit Canet <address@hidden>
> >---
> >  block.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index 749835c..71349e5 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -1846,11 +1846,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
> >*bs_dest,
> >      pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> >              bs_src->device_name);
> >      bs_dest->device_list = bs_src->device_list;
> >-
> >-    /* keep the same entry in graph_bdrv_states
> >-     * We do want to swap name but don't want to swap linked list entries
> >-     */
> >-    bs_dest->node_list   = bs_src->node_list;
> >  }
> >  /*
> >@@ -1869,6 +1864,17 @@ void bdrv_swap(BlockDriverState *bs_new, 
> >BlockDriverState *bs_old)
> >  {
> >      BlockDriverState tmp;
> >+    /* The code need to swap the node_name but simply swapping node_list 
> >won't
> 
> *needs
> 
> >+     * work so first remove the nodes from the graph list, do the swap then
> >+     * insert them back if needed.
> >+     */
> >+    if (bs_new->node_name[0] != '\0') {
> >+        QTAILQ_REMOVE(&graph_bdrv_states, bs_new, node_list);
> >+    }
> >+    if (bs_old->node_name[0] != '\0') {
> >+        QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list);
> >+    }
> >+
> >      /* bs_new must be anonymous and shouldn't have anything fancy enabled 
> > */
> >      assert(bs_new->device_name[0] == '\0');
> >      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
> >@@ -1897,6 +1903,14 @@ void bdrv_swap(BlockDriverState *bs_new, 
> >BlockDriverState *bs_old)
> >      assert(bs_new->io_limits_enabled == false);
> >      assert(!throttle_have_timer(&bs_new->throttle_state));
> >+    /* insert back the nodes in the graph node list if needed */
> 
> The word order seems strange, I guess I'd put the "back" behind
> "nodes" (and maybe "into" instead of "in").
> 
> >+    if (bs_new->node_name[0] != '\0') {
> >+        QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs_new, node_list);
> >+    }
> >+    if (bs_old->node_name[0] != '\0') {
> >+        QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs_old, node_list);
> >+    }
> >+
> >      bdrv_rebind(bs_new);
> >      bdrv_rebind(bs_old);
> >  }
> 
> Reviewed-by: Max Reitz <address@hidden>
> 

Thanks a lot I'll respin while adding your reviewed-by.

Best regards

Benoît