[PATCH v2 0/5] block permission updated follow-up

[PATCH v2 0/5] block permission updated follow-up

[Vladimir Sementsov-Ogievskiy]

v2: rebased on Kevin's "[PATCH 0/2] block: Fix Transaction leaks"
1: add assertions and drop extra declaration
2: add Alberto's r-b
3: improve commit message

Based-on: <20210503110555.24001-1-kwolf@redhat.com>

Vladimir Sementsov-Ogievskiy (5):
  block: document child argument of bdrv_attach_child_common()
  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                               | 61 +++++++++++++++++----------
 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, 46 insertions(+), 30 deletions(-)

-- 
2.29.2

[PATCH v2 1/5] 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.

So, let's add documentation and some assertions.

While being here, drop extra declaration of bdrv_attach_child_noperm().

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

diff --git a/block.c b/block.c
index 69615fabd1..b9df90d61d 100644
--- a/block.c
+++ b/block.c
@@ -85,14 +85,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 
 static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs);
-static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
-                                    BlockDriverState *child_bs,
-                                    const char *child_name,
-                                    const BdrvChildClass *child_class,
-                                    BdrvChildRole child_role,
-                                    BdrvChild **child,
-                                    Transaction *tran,
-                                    Error **errp);
 static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
                                             Transaction *tran);
 
@@ -2762,6 +2754,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,
@@ -2836,6 +2834,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,
@@ -2918,7 +2920,6 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                    child_role, perm, shared_perm, opaque,
                                    &child, tran, errp);
     if (ret < 0) {
-        assert(child == NULL);
         goto out;
     }
 
@@ -2926,6 +2927,9 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
 
 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;
 }
@@ -2965,6 +2969,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
 out:
     tran_finalize(tran, ret);
+    /* child is unset on failure by bdrv_attach_child_common_abort() */
+    assert((ret < 0) == !child);
 
     bdrv_unref(child_bs);
 
-- 
2.29.2

[PATCH v2 2/5] 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>
Reviewed-by: Alberto Garcia <berto@igalia.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

[PATCH v2 3/5] 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.

Next, 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 refers 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 b9df90d61d..54a3da9311 100644
--- a/block.c
+++ b/block.c
@@ -1152,7 +1152,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

[PATCH v2 4/5] 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 54a3da9311..2f73523285 100644
--- a/block.c
+++ b/block.c
@@ -2029,11 +2029,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

[PATCH v2 5/5] 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 2f73523285..354438d918 100644
--- a/block.c
+++ b/block.c
@@ -2032,20 +2032,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

Re: [PATCH v2 3/5] block: improve bdrv_child_get_parent_desc()

[Alberto Garcia]

On Tue 04 May 2021 11:45:08 AM 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.
>
> Next, 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 refers 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>

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

Berto

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

[Alberto Garcia]

On Tue 04 May 2021 11:45:09 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.

s/has/have/ , and I'm not sure what "have the realization" means

>  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);
>  }

Should we also assert(c->klass->get_parent_desc) ?

Berto

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

[Vladimir Sementsov-Ogievskiy]

10.05.2021 18:33, Alberto Garcia wrote:
> On Tue 04 May 2021 11:45:09 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> All existing parent types (block nodes, block devices, jobs) has the
>> realization. So, drop unreachable code.
> 
> s/has/have/ , and I'm not sure what "have the realization" means
> 
>>   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);
>>   }
> 
> Should we also assert(c->klass->get_parent_desc) ?
> 

No, as crash on calling zero pointer is practically not worse than crash on failed assertion.


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] block permission updated follow-up

[Vladimir Sementsov-Ogievskiy]

Ping. Now based on master

04.05.2021 12:45, Vladimir Sementsov-Ogievskiy wrote:
> v2: rebased on Kevin's "[PATCH 0/2] block: Fix Transaction leaks"
> 1: add assertions and drop extra declaration
> 2: add Alberto's r-b
> 3: improve commit message
> 
> Based-on: <20210503110555.24001-1-kwolf@redhat.com>
> 
> Vladimir Sementsov-Ogievskiy (5):
>    block: document child argument of bdrv_attach_child_common()
>    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                               | 61 +++++++++++++++++----------
>   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, 46 insertions(+), 30 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/5] block-backend: improve blk_root_get_parent_desc()

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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>
> ---
>  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");

We should probably keep the article: "an unnamed block device"

>      }
>  }

Kevin

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

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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>

Your fixes the other days showed that vvfat has a BdrvChildClass, too.
This instance doesn't define .get_parent_desc(), so the code that this
patch removes is potentially still reachable.

Kevin

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Kevin Wolf]

Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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 2f73523285..354438d918 100644
> --- a/block.c
> +++ b/block.c
> @@ -2032,20 +2032,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);

I think the combination of a_against and b_perm is confusing to report
because one is the intersection of permissions (i.e. only the
permissions that actually conflict) and the other the full list of
unshared permissions.

We could report both the full list of required permissions (which is
what your current error message claims to report) and of unshared
permissions. I'm not sure if there is actually any use for this
information.

The other option that would feel consistent is to report only the
conflicting permissions, and report them only once because they are the
same for both sides.

Kevin

>  
>      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
> 

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Vladimir Sementsov-Ogievskiy]

31.05.2021 19:07, Kevin Wolf wrote:
> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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 2f73523285..354438d918 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2032,20 +2032,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);
> 
> I think the combination of a_against and b_perm is confusing to report
> because one is the intersection of permissions (i.e. only the
> permissions that actually conflict) and the other the full list of
> unshared permissions.
> 
> We could report both the full list of required permissions (which is
> what your current error message claims to report) and of unshared
> permissions. I'm not sure if there is actually any use for this
> information.
> 
> The other option that would feel consistent is to report only the
> conflicting permissions, and report them only once because they are the
> same for both sides.
> 

Agreed.

So, what about:

   error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name);

?


> 
>>   
>>       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
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/5] block-backend: improve blk_root_get_parent_desc()

[Vladimir Sementsov-Ogievskiy]

31.05.2021 18:45, Kevin Wolf wrote:
> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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>
>> ---
>>   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");
> 
> We should probably keep the article: "an unnamed block device"

OK, will do


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

31.05.2021 18:50, Kevin Wolf wrote:
> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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>
> 
> Your fixes the other days showed that vvfat has a BdrvChildClass, too.
> This instance doesn't define .get_parent_desc(), so the code that this
> patch removes is potentially still reachable.
> 

Right. Will fix it and add an assertion to bdrv_attach_child_common()


-- 
Best regards,
Vladimir

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Kevin Wolf]

Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 31.05.2021 19:07, Kevin Wolf wrote:
> > Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 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 2f73523285..354438d918 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -2032,20 +2032,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);
> > 
> > I think the combination of a_against and b_perm is confusing to report
> > because one is the intersection of permissions (i.e. only the
> > permissions that actually conflict) and the other the full list of
> > unshared permissions.
> > 
> > We could report both the full list of required permissions (which is
> > what your current error message claims to report) and of unshared
> > permissions. I'm not sure if there is actually any use for this
> > information.
> > 
> > The other option that would feel consistent is to report only the
> > conflicting permissions, and report them only once because they are the
> > same for both sides.
> > 
> 
> Agreed.
> 
> So, what about:
> 
>   error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name);

I'm not sure if I'm happy with the child names simply in parentheses,
but I don't have a good alternative. I was thinking something like
"(node used as %s)", but while writing down the example below, that
turned out confusing because a_user and b_user can refer to nodes, too.

"permissions '%s'" with single quotes might be preferable, too.

So a real error message from the current version of the patch is:

    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

It would then become:

    Permission conflict on node 'base': permissions 'write' are both
    required by node 'other' (image) and unshared by 'source' (image).

Looks like an improvement to me, but if anyone has a good idea what to
do about the unclear meaning of the parentheses, I would be happy to
hear suggestions.

Kevin

> > >       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
> > > 
> > 
> 
> 
> -- 
> Best regards,
> Vladimir
> 

Re: [PATCH v2 5/5] block: improve permission conflict error message

[Vladimir Sementsov-Ogievskiy]

31.05.2021 19:35, Kevin Wolf wrote:
> Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 31.05.2021 19:07, Kevin Wolf wrote:
>>> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 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 2f73523285..354438d918 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -2032,20 +2032,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);
>>>
>>> I think the combination of a_against and b_perm is confusing to report
>>> because one is the intersection of permissions (i.e. only the
>>> permissions that actually conflict) and the other the full list of
>>> unshared permissions.
>>>
>>> We could report both the full list of required permissions (which is
>>> what your current error message claims to report) and of unshared
>>> permissions. I'm not sure if there is actually any use for this
>>> information.
>>>
>>> The other option that would feel consistent is to report only the
>>> conflicting permissions, and report them only once because they are the
>>> same for both sides.
>>>
>>
>> Agreed.
>>
>> So, what about:
>>
>>    error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name);
> 
> I'm not sure if I'm happy with the child names simply in parentheses,
> but I don't have a good alternative. I was thinking something like
> "(node used as %s)", but while writing down the example below, that
> turned out confusing because a_user and b_user can refer to nodes, too.
> 
> "permissions '%s'" with single quotes might be preferable, too.
> 
> So a real error message from the current version of the patch is:
> 
>      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
> 
> It would then become:
> 
>      Permission conflict on node 'base': permissions 'write' are both
>      required by node 'other' (image) and unshared by 'source' (image).
> 
> Looks like an improvement to me, but if anyone has a good idea what to
> do about the unclear meaning of the parentheses, I would be happy to
> hear suggestions.
> 

The only idea I have is duplicating (hmm, "triplicating" is an existing word?) the node of conflict:

bs_n = bdrv_get_node_name(b->bs);

error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (uses node '%s' as '%s' child) and unshared by %s (uses node '%s' as '%s' child).", bs_n, a_against, b_user, bs_n, b->name, a_user, bs_n, a->name);

-- 
Best regards,
Vladimir