[PATCH 0/6] block permission updated follow-up

[PATCH 0/6] block permission updated follow-up

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are two coverity fixes and improvement of error message.

Vladimir Sementsov-Ogievskiy (6):
  block: fix leak of tran in bdrv_root_attach_child
  block: bdrv_reopen_multiple(): fix leak of tran object
  block-backend: improve blk_root_get_parent_desc()
  block: improve bdrv_child_get_parent_desc()
  block: simplify bdrv_child_user_desc()
  block: improve permission conflict error message

 block.c                               | 51 ++++++++++++++++++---------
 block/block-backend.c                 |  9 +++--
 tests/qemu-iotests/283.out            |  2 +-
 tests/qemu-iotests/307.out            |  2 +-
 tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
 5 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.29.2

[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child

[Vladimir Sementsov-Ogievskiy]

bdrv_attach_child_common() doesn't require tran_finalize() on failure
(it does tran_add() only on success path). Still tran_new() must be
paired with tran_finalize() anyway, at least to free empty Transaction
object itself.

So, refactor the function for clean finalization code, same on all
paths.

While being here, also leave a comment on unobvious background zeroing
of child pointer on failure path.

Reported-by: Coverity (CID 1452773)
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 874c22c43e..728aa34b2f 100644
--- a/block.c
+++ b/block.c
@@ -2918,12 +2918,18 @@ 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;
+        goto out;
     }
 
     ret = bdrv_refresh_perms(child_bs, errp);
+    if (ret < 0) {
+        goto out;
+    }
+
+out:
     tran_finalize(tran, ret);
+    /* child is unset on failure by bdrv_attach_child_common_abort() */
+    assert((ret < 0) == !child);
 
     bdrv_unref(child_bs);
     return child;
-- 
2.29.2

Re: [PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child

[Alberto Garcia]

On Mon 03 May 2021 01:33:57 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> @@ -2918,12 +2918,18 @@ 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;
> +        goto out;
>      }
>  
>      ret = bdrv_refresh_perms(child_bs, errp);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +out:

I see why you're doing this last error check, but it looks a bit
weird. My first reaction was to think that I was missing something.

I would remove it.

Berto

Re: [PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child

[Vladimir Sementsov-Ogievskiy]

03.05.2021 18:51, Alberto Garcia wrote:
> On Mon 03 May 2021 01:33:57 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> @@ -2918,12 +2918,18 @@ 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;
>> +        goto out;
>>       }
>>   
>>       ret = bdrv_refresh_perms(child_bs, errp);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>> +out:
> 
> I see why you're doing this last error check, but it looks a bit
> weird. My first reaction was to think that I was missing something.
> 
> I would remove it.
> 

Hmm. I don't know. And don't insist of course.


-- 
Best regards,
Vladimir

[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object

[Vladimir Sementsov-Ogievskiy]

We have one path, where tran object is created, but we don't touch and
don't free it in any way: "goto cleanup" in first loop with calls to
bdrv_flush().

Fix it simply moving tran_new() call below that loop.

Reported-by: Coverity (CID 1452772)
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 728aa34b2f..c4023ab4f4 100644
--- a/block.c
+++ b/block.c
@@ -4047,7 +4047,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 {
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
-    Transaction *tran = tran_new();
+    Transaction *tran;
     g_autoptr(GHashTable) found = NULL;
     g_autoptr(GSList) refresh_list = NULL;
 
@@ -4061,6 +4061,8 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
         }
     }
 
+    tran = tran_new();
+
     QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         assert(bs_entry->state.bs->quiesce_counter > 0);
         ret = bdrv_reopen_prepare(&bs_entry->state, bs_queue, tran, errp);
-- 
2.29.2

Re: [PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object

[Alberto Garcia]

On Mon 03 May 2021 01:33:58 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> We have one path, where tran object is created, but we don't touch and
> don't free it in any way: "goto cleanup" in first loop with calls to
> bdrv_flush().
>
> Fix it simply moving tran_new() call below that loop.
>
> Reported-by: Coverity (CID 1452772)
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object

[Vladimir Sementsov-Ogievskiy]

03.05.2021 18:52, Alberto Garcia wrote:
> On Mon 03 May 2021 01:33:58 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> We have one path, where tran object is created, but we don't touch and
>> don't free it in any way: "goto cleanup" in first loop with calls to
>> bdrv_flush().
>>
>> Fix it simply moving tran_new() call below that loop.
>>
>> Reported-by: Coverity (CID 1452772)
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 

Thanks!  Still, now I see that Kevin's patch is better ([PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple())


-- 
Best regards,
Vladimir

[PATCH 3/6] block-backend: improve blk_root_get_parent_desc()

[Vladimir Sementsov-Ogievskiy]

We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.

While being here also use g_autofree.

iotest 307 output is updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-backend.c      | 9 ++++-----
 tests/qemu-iotests/307.out | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6fca9853e1..2b7e9b5192 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -142,19 +142,18 @@ static void blk_root_set_aio_ctx(BdrvChild *child, AioContext *ctx,
 static char *blk_root_get_parent_desc(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
-    char *dev_id;
+    g_autofree char *dev_id = NULL;
 
     if (blk->name) {
-        return g_strdup(blk->name);
+        return g_strdup_printf("block device '%s'", blk->name);
     }
 
     dev_id = blk_get_attached_dev_id(blk);
     if (*dev_id) {
-        return dev_id;
+        return g_strdup_printf("block device '%s'", dev_id);
     } else {
         /* TODO Callback into the BB owner for something more detailed */
-        g_free(dev_id);
-        return g_strdup("a block device");
+        return g_strdup("unnamed block device");
     }
 }
 
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index daa8ad2da0..66bf2ddb74 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -53,7 +53,7 @@ exports available: 1
 
 === Add a writable export ===
 {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by sda as 'root', which does not allow 'write' on fmt"}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}}
 {"execute": "device_del", "arguments": {"id": "sda"}}
 {"return": {}}
 {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-- 
2.29.2

Re: [PATCH 3/6] block-backend: improve blk_root_get_parent_desc()

[Alberto Garcia]

On Mon 03 May 2021 01:33:59 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> We have different types of parents: block nodes, block backends and
> jobs. So, it makes sense to specify type together with name.
>
> While being here also use g_autofree.
>
> iotest 307 output is updated.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[PATCH 4/6] block: improve bdrv_child_get_parent_desc()

[Vladimir Sementsov-Ogievskiy]

We have different types of parents: block nodes, block backends and
jobs. So, it makes sense to specify type together with name.

iotest 283 output is updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                    | 2 +-
 tests/qemu-iotests/283.out | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index c4023ab4f4..1de2365843 100644
--- a/block.c
+++ b/block.c
@@ -1160,7 +1160,7 @@ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)
 static char *bdrv_child_get_parent_desc(BdrvChild *c)
 {
     BlockDriverState *parent = c->opaque;
-    return g_strdup(bdrv_get_device_or_node_name(parent));
+    return g_strdup_printf("node '%s'", bdrv_get_node_name(parent));
 }
 
 static void bdrv_child_cb_drained_begin(BdrvChild *child)
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index 97e62a4c94..c9397bfc44 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
 {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}
 
 === backup-top should be gone after job-finalize ===
 
-- 
2.29.2

Re: [PATCH 4/6] block: improve bdrv_child_get_parent_desc()

[Vladimir Sementsov-Ogievskiy]

03.05.2021 14:34, Vladimir Sementsov-Ogievskiy wrote:
> We have different types of parents: block nodes, block backends and
> jobs. So, it makes sense to specify type together with name.

I forget to note the main thing of the commit:

This handler us used to compose an error message about permission conflict. And permission conflict occurs in a specific place of block graph. We shouldn't report name of parent device (as it would define another place in block graph), but exactly and only the name of the node. So, use bdrv_get_node_name() directly.

> 
> iotest 283 output is updated.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c                    | 2 +-
>   tests/qemu-iotests/283.out | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c4023ab4f4..1de2365843 100644
> --- a/block.c
> +++ b/block.c
> @@ -1160,7 +1160,7 @@ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)
>   static char *bdrv_child_get_parent_desc(BdrvChild *c)
>   {
>       BlockDriverState *parent = c->opaque;
> -    return g_strdup(bdrv_get_device_or_node_name(parent));
> +    return g_strdup_printf("node '%s'", bdrv_get_node_name(parent));
>   }
>   
>   static void bdrv_child_cb_drained_begin(BdrvChild *child)
> diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
> index 97e62a4c94..c9397bfc44 100644
> --- a/tests/qemu-iotests/283.out
> +++ b/tests/qemu-iotests/283.out
> @@ -5,7 +5,7 @@
>   {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
>   {"return": {}}
>   {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}
> -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}}
> +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}
>   
>   === backup-top should be gone after job-finalize ===
>   
> 


-- 
Best regards,
Vladimir

[PATCH 5/6] block: simplify bdrv_child_user_desc()

[Vladimir Sementsov-Ogievskiy]

All existing parent types (block nodes, block devices, jobs) has the
realization. So, drop unreachable code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 1de2365843..20efd7a7b0 100644
--- a/block.c
+++ b/block.c
@@ -2037,11 +2037,7 @@ bool bdrv_is_writable(BlockDriverState *bs)
 
 static char *bdrv_child_user_desc(BdrvChild *c)
 {
-    if (c->klass->get_parent_desc) {
-        return c->klass->get_parent_desc(c);
-    }
-
-    return g_strdup("another user");
+    return c->klass->get_parent_desc(c);
 }
 
 static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
-- 
2.29.2

Re: [PATCH 5/6] block: simplify bdrv_child_user_desc()

[Alberto Garcia]

On Mon 03 May 2021 01:34:01 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

With the updated description that you propose in your reply to the
patch,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 5/6] block: simplify bdrv_child_user_desc()

[Vladimir Sementsov-Ogievskiy]

03.05.2021 19:05, Alberto Garcia wrote:
> On Mon 03 May 2021 01:34:01 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> All existing parent types (block nodes, block devices, jobs) has the
>> realization. So, drop unreachable code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> With the updated description that you propose in your reply to the
> patch,

Hmm, is it answer to 4/6, not 5/6 ?

> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 


-- 
Best regards,
Vladimir

[PATCH 6/6] block: improve permission conflict error message

[Vladimir Sementsov-Ogievskiy]

Now permissions are updated as follows:
 1. do graph modifications ignoring permissions
 2. do permission update

 (of course, we rollback [1] if [2] fails)

So, on stage [2] we can't say which users are "old" and which are
"new" and exist only since [1]. And current error message is a bit
outdated. Let's improve it, to make everything clean.

While being here, add also a comment and some good assertions.

iotests 283, 307, qsd-jobs outputs are updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                               | 29 ++++++++++++++++++++-------
 tests/qemu-iotests/283.out            |  2 +-
 tests/qemu-iotests/307.out            |  2 +-
 tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 20efd7a7b0..3fc87fbf90 100644
--- a/block.c
+++ b/block.c
@@ -2040,20 +2040,35 @@ static char *bdrv_child_user_desc(BdrvChild *c)
     return c->klass->get_parent_desc(c);
 }
 
+/*
+ * Check that @a allows everything that @b needs. @a and @b must reference same
+ * child node.
+ */
 static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
 {
-    g_autofree char *user = NULL;
-    g_autofree char *perm_names = NULL;
+    g_autofree char *a_user = NULL;
+    g_autofree char *a_against = NULL;
+    g_autofree char *b_user = NULL;
+    g_autofree char *b_perm = NULL;
+
+    assert(a->bs);
+    assert(a->bs == b->bs);
 
     if ((b->perm & a->shared_perm) == b->perm) {
         return true;
     }
 
-    perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);
-    user = bdrv_child_user_desc(a);
-    error_setg(errp, "Conflicts with use by %s as '%s', which does not "
-               "allow '%s' on %s",
-               user, a->name, perm_names, bdrv_get_node_name(b->bs));
+    a_user = bdrv_child_user_desc(a);
+    a_against = bdrv_perm_names(b->perm & ~a->shared_perm);
+
+    b_user = bdrv_child_user_desc(b);
+    b_perm = bdrv_perm_names(b->perm);
+
+    error_setg(errp, "Permission conflict on node '%s': %s wants to use it as "
+               "'%s', which requires these permissions: %s. On the other hand %s "
+               "wants to use it as '%s', which doesn't share: %s",
+               bdrv_get_node_name(b->bs),
+               b_user, b->name, b_perm, a_user, a->name, a_against);
 
     return false;
 }
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index c9397bfc44..92f3cc1ed5 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -5,7 +5,7 @@
 {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}
-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}
+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write"}}
 
 === backup-top should be gone after job-finalize ===
 
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index 66bf2ddb74..e03932ba4f 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -53,7 +53,7 @@ exports available: 1
 
 === Add a writable export ===
 {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand block device 'sda' wants to use it as 'root', which doesn't share: write"}}
 {"execute": "device_del", "arguments": {"id": "sda"}}
 {"return": {}}
 {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out
index 9f52255da8..b0596d2c95 100644
--- a/tests/qemu-iotests/tests/qsd-jobs.out
+++ b/tests/qemu-iotests/tests/qsd-jobs.out
@@ -16,7 +16,7 @@ QMP_VERSION
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
-{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand stream job 'job0' wants to use it as 'intermediate node', which doesn't share: write"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}}
 *** done
-- 
2.29.2

[PATCH 7/6] block: document child argument of bdrv_attach_child_common()

[Vladimir Sementsov-Ogievskiy]

The logic around **child is not obvious: this reference is used not
only to return resulting child, but also to rollback NULL value on
transaction abort. Let's document this.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block.c b/block.c
index 3fc87fbf90..1f89be6f97 100644
--- a/block.c
+++ b/block.c
@@ -2773,6 +2773,12 @@ static TransactionActionDrv bdrv_attach_child_common_drv = {
 
 /*
  * Common part of attaching bdrv child to bs or to blk or to job
+ *
+ * Resulting new child is returned through @child.
+ * At start *@child must be NULL.
+ * @child is saved to a new entry of @tran, so that *@child could be reverted to
+ * NULL on abort(). So referenced variable must live at least until transaction
+ * end.
  */
 static int bdrv_attach_child_common(BlockDriverState *child_bs,
                                     const char *child_name,
@@ -2847,6 +2853,10 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
     return 0;
 }
 
+/*
+ * Variable referenced by @child must live at least until transaction end.
+ * (see bdrv_attach_child_common() doc for details)
+ */
 static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
                                     BlockDriverState *child_bs,
                                     const char *child_name,
-- 
2.29.2