Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()

[Max Reitz <mreitz@redhat.com>]

On 03.05.21 13:05, Kevin Wolf wrote:
> The error path needs to call tran_finalize(), too.
> 
> Fixes: CID 1452773
> Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 874c22c43e..5c0ced6238 100644
> --- a/block.c
> +++ b/block.c
> @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>                                      child_role, perm, shared_perm, opaque,
>                                      &child, tran, errp);
>       if (ret < 0) {
> -        bdrv_unref(child_bs);
> -        return NULL;
> +        assert(child == NULL);
> +        goto out;
>       }
>   
>       ret = bdrv_refresh_perms(child_bs, errp);
> -    tran_finalize(tran, ret);
>   
> +out:
> +    tran_finalize(tran, ret);
>       bdrv_unref(child_bs);
>       return child;

Looks OK, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

However, the function’s description says that it will return NULL on 
error.  But if bdrv_refresh_perms() fails, it will still return a 
non-NULL child.  Is that right?