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

[Max Reitz <mreitz@redhat.com>]

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>