03.05.2021 15:14, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:53, Max Reitz wrote:
On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote:
03.05.2021 14:49, Max Reitz wrote:
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 <kw...@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 <mre...@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?
No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and
I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix
leak of tran in bdrv_root_attach_child"
The fact that the transaction keeps the pointer to this local variable around
is a bit horrifying, but well.
Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm
where we would have to add a separate entry to @tran to rollback bs->backing
change. Probably, it's better to refactor this thing. Or at least document that
out-pointer argument goes into transaction, and should be valid up to transaction
finalization.
documentation done:
[PATCH 7/6] block: document child argument of bdrv_attach_child_common()
--
Best regards,
Vladimir
