Re: [PATCH v2 05/10] block: Pass BdrvChild ** to replace_child_noperm

[Vladimir Sementsov-Ogievskiy]

11.11.2021 15:08, Hanna Reitz wrote:
> bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
> set it to NULL.  That is dangerous, because BDS parents generally assume
> that their children's .bs pointer is never NULL.  We therefore want to
> let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
> to NULL, too.
>
> This patch lays the foundation for it by passing a BdrvChild ** pointer
> to bdrv_replace_child_noperm() so that it can later use it to NULL the
> BdrvChild pointer immediately after setting BdrvChild.bs to NULL.
>
> (We will still need to undertake some intermediate steps, though.)
>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Series already applied, but I still feel myself responsible to track how 
transactions changed:)

Don't bother with applying my r-b marks into applied series.

> ---
>   block.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/block.c b/block.c
> index c7d5aa5254..d668156eca 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>   static bool bdrv_recurse_has_child(BlockDriverState *bs,
>                                      BlockDriverState *child);
>   
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **child,
>                                         BlockDriverState *new_bs);
>   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>                                                 BdrvChild *child,
> @@ -2270,7 +2270,7 @@ static void bdrv_replace_child_abort(void *opaque)
>       BlockDriverState *new_bs = s->child->bs;
>   
>       /* old_bs reference is transparently moved from @s to @s->child */
> -    bdrv_replace_child_noperm(s->child, s->old_bs);
> +    bdrv_replace_child_noperm(&s->child, s->old_bs);

 - no sense / no harm in  clearing the pointer, as it's a field in transaction 
state struct, and should not be used after abort
 - hard to say do we really need clearing some another pointer, upper level 
should care about it

>       bdrv_unref(new_bs);
>   }
>   
> @@ -2300,7 +2300,7 @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
>       if (new_bs) {
>           bdrv_ref(new_bs);
>       }
> -    bdrv_replace_child_noperm(child, new_bs);
> +    bdrv_replace_child_noperm(&child, new_bs);

 - no sence / no harm, as it's a local variable, which is not used anymore
 - most probably we have some another pointer that should be cleared, but it's 
not available here.. To make it available, bdrv_replace_child_tran() should get 
BdrvChild **.. maybe later patch will do it

>       /* old_bs reference is transparently moved from @child to @s */
>   }
>   
> @@ -2672,9 +2672,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
>       return permissions[qapi_perm];
>   }
>   
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **childp,
>                                         BlockDriverState *new_bs)
>   {
> +    BdrvChild *child = *childp;

No real logic change for now, OK

>       BlockDriverState *old_bs = child->bs;
>       int new_bs_quiesce_counter;
>       int drain_saldo;
> @@ -2767,7 +2768,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
>       BdrvChild *child = *s->child;
>       BlockDriverState *bs = child->bs;
>   
> -    bdrv_replace_child_noperm(child, NULL);
> +    bdrv_replace_child_noperm(s->child, NULL);

More interesting. Currently bdrv_replace_child_tran() clear the pointer as last action, so 
later we can remove this last "*s->child = NULL" as bdrv_replace_child_noperm() 
will do it.
No harm: in the the function we use local variable, initialized as *s->child.

>   
>       if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
>           bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
> @@ -2867,7 +2868,7 @@ static int bdrv_attach_child_common(BlockDriverState 
> *child_bs,
>       }
>   
>       bdrv_ref(child_bs);
> -    bdrv_replace_child_noperm(new_child, child_bs);
> +    bdrv_replace_child_noperm(&new_child, child_bs);

Here child_bs must not be NULL, otherwise bdrv_ref() crashes. So, nothing would 
be cleared.

>   
>       *child = new_child;
>   
> @@ -2922,12 +2923,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
>       return 0;
>   }
>   
> -static void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild **childp)
>   {
> -    BlockDriverState *old_bs = child->bs;
> +    BlockDriverState *old_bs = (*childp)->bs;
>   
> -    bdrv_replace_child_noperm(child, NULL);
> -    bdrv_child_free(child);
> +    bdrv_replace_child_noperm(childp, NULL);

And here for sure we'll clear the pointer

> +    bdrv_child_free(*childp);

This obviously should be changed in further patches

>   
>       if (old_bs) {
>           /*
> @@ -3033,7 +3034,7 @@ void bdrv_root_unref_child(BdrvChild *child)
>       BlockDriverState *child_bs;
>   
>       child_bs = child->bs;
> -    bdrv_detach_child(child);
> +    bdrv_detach_child(&child);

 - no sence / no harm, as it's a local variable, which is not used anymore

>       bdrv_unref(child_bs);
>   }
>   

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

--
Best regards,
Vladimir