qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 18/39] block: add bdrv_attach_child_common() transaction actio


From: Peter Maydell
Subject: Re: [PULL 18/39] block: add bdrv_attach_child_common() transaction action
Date: Fri, 30 Apr 2021 23:33:12 +0100

On Fri, 30 Apr 2021 at 11:53, Kevin Wolf <kwolf@redhat.com> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Split out no-perm part of bdrv_root_attach_child() into separate
> transaction action. bdrv_root_attach_child() now moves to new
> permission update paradigm: first update graph relations then update
> permissions.
>
> qsd-jobs test output updated. Seems now permission update goes in
> another order. Still, the test comment say that we only want to check
> that command doesn't crash, and it's still so.
>
> Error message is a bit misleading as it looks like job was added first.
> But actually in new paradigm of graph update we can't distinguish such
> things. We should update the error message, but let's not do it now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Message-Id: <20210428151804.439460-19-vsementsov@virtuozzo.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Hi; Coverity thinks it's found a resource leak in this function
(CID 1452773):

> +/*
> + * This function steals the reference to child_bs from the caller.
> + * That reference is later dropped by bdrv_root_unref_child().
> + *
> + * On failure NULL is returned, errp is set and the reference to
> + * child_bs is also dropped.
> + *
> + * The caller must hold the AioContext lock @child_bs, but not that of @ctx
> + * (unless @child_bs is already in @ctx).
> + */
> +BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
> +                                  const char *child_name,
> +                                  const BdrvChildClass *child_class,
> +                                  BdrvChildRole child_role,
> +                                  uint64_t perm, uint64_t shared_perm,
> +                                  void *opaque, Error **errp)
> +{
> +    int ret;
> +    BdrvChild *child = NULL;
> +    Transaction *tran = tran_new();

Here we create a new Transaction...

> +
> +    ret = bdrv_attach_child_common(child_bs, child_name, child_class,
> +                                   child_role, perm, shared_perm, opaque,
> +                                   &child, tran, errp);
> +    if (ret < 0) {
> +        bdrv_unref(child_bs);
> +        return NULL;

...but in this error-return path we don't free it.

> +    }
> +
> +    ret = bdrv_refresh_perms(child_bs, errp);
> +    tran_finalize(tran, ret);
> +
> +    bdrv_unref(child_bs);
>      return child;
>  }

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]