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