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
>