Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben: > As it already said in the comment, we don't want to create loops in > parent->child relations. So, when we try to append @to to @c, we should > check that @c is not in @to children subtree, and we should check it > recursively, not only the first level. The patch provides BFS-based > search, to check the relations. > > This is needed for further fleecing-hook filter usage: we need to > append it to source, when the hook is already a parent of target, and > source may be in a backing chain of target (fleecing-scheme). So, on > appending, the hook should not became a child (direct or through > children subtree) of the target. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > block.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index c298ca6a19..7f605b0bf0 100644 > --- a/block.c > +++ b/block.c > @@ -3432,7 +3432,7 @@ void bdrv_close_all(void) > > static bool should_update_child(BdrvChild *c, BlockDriverState *to) > { > - BdrvChild *to_c; > + GList *queue = NULL, *pos; > > if (c->role->stay_at_node) { > return false; > @@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c, > BlockDriverState *to) > * if A is a child of B, that means we cannot replace A by B there > * because that would create a loop. Silently detaching A from B > * is also not really an option. So overall just leaving A in > - * place there is the most sensible choice. */ > - QLIST_FOREACH(to_c, &to->children, next) { > - if (to_c == c) { > - return false; > + * place there is the most sensible choice. > + * > + * upd: If the child @c belongs to the @to's children, or children of > it's > + * children and so on - this would create a loop to. To prevent it let's > do > + * a BFS search on @to children subtree. > + */
I don't like the "upd:" thing. Let me try to rephrase the addition: We would also create a loop in any cases where @c is only indirectly referenced by @to. Prevent this by returning false if @c is found (by breadth-first search) anywhere in the whole subtree of @to. Also, even though the coding style says 80 characters per line, the existing comment seems to use only 70 characters. Let's try to stay consistent here, otherwise it looks a bit odd. > + pos = queue = g_list_append(queue, to); > + while (pos) { > + BlockDriverState *v = pos->data; > + BdrvChild *c2; > + > + QLIST_FOREACH(c2, &v->children, next) { > + if (c2 == c) { > + g_list_free(queue); > + return false; > + } > + > + if (g_list_find(queue, c2->bs)) { > + continue; > + } > + > + queue = g_list_append(queue, c2->bs); > } > + > + pos = pos->next; > } > > + g_list_free(queue); > return true; > } The functional change looks good to me. Kevin