[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
- [PULL 08/39] util: add transactions.c, (continued)
- [PULL 08/39] util: add transactions.c, Kevin Wolf, 2021/04/30
- [PULL 10/39] block: refactor bdrv_child* permission functions, Kevin Wolf, 2021/04/30
- [PULL 11/39] block: rewrite bdrv_child_try_set_perm() using bdrv_refresh_perms(), Kevin Wolf, 2021/04/30
- [PULL 09/39] block: bdrv_refresh_perms: check for parents permissions conflict, Kevin Wolf, 2021/04/30
- [PULL 12/39] block: inline bdrv_child_*() permission functions calls, Kevin Wolf, 2021/04/30
- [PULL 13/39] block: use topological sort for permission update, Kevin Wolf, 2021/04/30
- [PULL 14/39] block: add bdrv_drv_set_perm transaction action, Kevin Wolf, 2021/04/30
- [PULL 15/39] block: add bdrv_list_* permission update functions, Kevin Wolf, 2021/04/30
- [PULL 17/39] block: fix bdrv_replace_node_common, Kevin Wolf, 2021/04/30
- [PULL 18/39] block: add bdrv_attach_child_common() transaction action, Kevin Wolf, 2021/04/30
- Re: [PULL 18/39] block: add bdrv_attach_child_common() transaction action,
Peter Maydell <=
- [PULL 21/39] block: adapt bdrv_append() for inserting filters, Kevin Wolf, 2021/04/30
- [PULL 20/39] block: split out bdrv_replace_node_noperm(), Kevin Wolf, 2021/04/30
- [PULL 23/39] block: introduce bdrv_drop_filter(), Kevin Wolf, 2021/04/30
- [PULL 19/39] block: add bdrv_attach_child_noperm() transaction action, Kevin Wolf, 2021/04/30
- [PULL 22/39] block: add bdrv_remove_filter_or_cow transaction action, Kevin Wolf, 2021/04/30
- [PULL 26/39] block: make bdrv_unset_inherits_from to be a transaction action, Kevin Wolf, 2021/04/30
- [PULL 16/39] block: add bdrv_replace_child_safe() transaction action, Kevin Wolf, 2021/04/30
- [PULL 27/39] block: make bdrv_refresh_limits() to be a transaction action, Kevin Wolf, 2021/04/30
- [PULL 24/39] block/backup-top: drop .active, Kevin Wolf, 2021/04/30
- [PULL 25/39] block: drop ignore_children for permission update functions, Kevin Wolf, 2021/04/30