[Qemu-devel] [QEMU 2.0 Fix] Fix bdrv_swap behavior regarding the node graph

[Qemu-devel] [QEMU 2.0 Fix] Fix bdrv_swap behavior regarding the node graph

[Benoît Canet]

This bug was discovered while working on the quorum file replacement.

The patch switch to a more sane behavior discussed with Kevin.

Best regards

Benoît

Benoît Canet (1):
  block: make bdrv_swap rebuild the bs graph node list field.

 block.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
1.8.3.2

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

[Benoît Canet]

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 <benoit@irqsave.net>
---
 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
+     * 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 */
+    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);
 }
-- 
1.8.3.2

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

[Max Reitz]

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 <benoit@irqsave.net>
> ---
>   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 <mreitz@redhat.com>

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 <benoit@irqsave.net>
> >---
> >  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 <mreitz@redhat.com>
> 

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

Best regards

Benoît