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
> ---
> 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, >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, >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
