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

[Kevin Wolf <kwolf@redhat.com>]

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
>