Re: [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay

[Kevin Wolf <kwolf@redhat.com>]

Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
> change_parent_backing_link() asserts that the BDS to be replaced is not
> used as a backing file. However, we may want to replace a BDS by its
> overlay in which case that very link should not be redirected.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

So the scenario is like this?

              +---- to
              v
    base <- from <- top

And we want to change it into this:

    base <- from <- to <- top

Okay, makes sense.

(Hm, put ASCII art in the commit message? I'd be all for it.)

>  block.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index f54bc25..16463aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -2224,9 +2224,22 @@ void bdrv_close_all(void)
>  static void change_parent_backing_link(BlockDriverState *from,
>                                         BlockDriverState *to)
>  {
> -    BdrvChild *c, *next;
> +    BdrvChild *c, *next, *to_c;
>  
>      QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> +        if (c->role == &child_backing) {
> +            /* Allow @from to be in a backing chain, but only if it is @to's
> +             * backing chain. Do not replace @from by @to there. */

The comment suggests bdrv_chain_contains(), but you only accept it as a
direct child of top and you accept non-backing-file children.

Intuitively I would say that anywhere in the backing chain would make
sense, but we can always allow that later when we actually need it.
Accepting all types of children sounds right when I think about
inserting a quorum node as 'to'.

So I guess the code is fine and the comment should be changed to
correctly reflect what the code does.

> +            QLIST_FOREACH(to_c, &to->children, next) {
> +                if (to_c == c) {
> +                    break;
> +                }
> +            }
> +            if (to_c) {
> +                continue;
> +            }
> +        }
> +
>          assert(c->role != &child_backing);
>          bdrv_ref(to);
>          bdrv_replace_child(c, to);

The other thing is that I'm unsure whether this function makes any sense
at all. "Replace in all parents" is kind of arbitrary. In the long term,
we may want to allow the user to specify the exact graph modifications
on (block-)job-complete.

Kevin