Re: [PATCH 4/7] block: Drop detached child from ignore list

[Vladimir Sementsov-Ogievskiy via <qemu-devel@nongnu.org>]

04.11.2021 13:38, Hanna Reitz wrote:
> bdrv_attach_child_common_abort() restores the parent's AioContext.  To
> do so, the child (which was supposed to be attached, but is now detached
> again by this abort handler) is added to the ignore list for the
> AioContext changing functions.
> 
> However, since we modify a BDS's children list in the BdrvChildClass's
> .attach and .detach handlers, the child is already effectively detached
> from the parent by this point.  We do not need to put it into the ignore
> list.
> 
> Use this opportunity to clean up the empty line structure: Keep setting
> the ignore list, invoking the AioContext function, and freeing the
> ignore list in blocks separated by empty lines.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


What comes in my mind, is that now bdrv_replace_child_noperm() is more strong in detaching.. But may be not enough strong: we still have link from the child to parent (child->opaque).. Maybe more correct for BdrvChild object to have no relation with parent after detaching. But than we'll need some GenericParent object (as child->class is mostly about parent, not about child and even not about the edge).. Now GenericParent is "included" into BdrvChild, which represents both GenericParent and Edge objects..

> ---
>   block.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b95f8dcf8f..6d230ad3d1 100644
> --- a/block.c
> +++ b/block.c
> @@ -2774,14 +2774,16 @@ static void bdrv_attach_child_common_abort(void *opaque)
>       }
>   
>       if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
> -        GSList *ignore = g_slist_prepend(NULL, child);
> +        GSList *ignore;
>   
> +        /* No need to ignore `child`, because it has been detached already */
> +        ignore = NULL;
>           child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
>                                         &error_abort);
>           g_slist_free(ignore);
> -        ignore = g_slist_prepend(NULL, child);
> -        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
>   
> +        ignore = NULL;
> +        child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
>           g_slist_free(ignore);
>       }
>   
> 


-- 
Best regards,
Vladimir