[RFC,23/41] block: Include details on permission errors in message

Instead of just telling that there was some conflict, we can be specific
and tell which permissions were in conflict and which way the conflict
is.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 11 deletions(-)

On 13.02.2017 18:22, Kevin Wolf wrote:
> Instead of just telling that there was some conflict, we can be specific
> and tell which permissions were in conflict and which way the conflict
> is.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 2116542..d743f50 100644
> --- a/block.c
> +++ b/block.c
> @@ -1381,6 +1381,43 @@ static void bdrv_update_perm(BlockDriverState *bs)
>      bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
>  }
>  
> +static char *bdrv_child_link_name(BdrvChild *c)
> +{
> +    if (c->role->get_link_name) {
> +        return c->role->get_link_name(c);
> +    }
> +
> +    return g_strdup("another user");

...now I'm a bit disappointed, not least because "another user" is not a
link name.

But I don't think we want a link name anyway. I'd write it differently
altogether. Looking at the code below, this will be used as "Conflicts
with ${link_name}".

Now, "Conflicts with backing file link from foo" reads weird. In fact,
anything that actually reports a "link" is weird. I'd propose the following:

"Conflicts with [use by] ${user} (used as ${c->name})"

user is c->role->get_name() if that exists, or "another user" if it does
not. If you implemented get_name() instead of get_link_name() for the
backing file role, there wouldn't be any need for get_link_name() at
all, IMO.

Also, you can implement c->role->get_name() not only for child_backing,
but also for child_file and child_format.

Max

> +}
> +
> +static char *bdrv_perm_names(uint64_t perm)
> +{
> +    struct perm_name {
> +        uint64_t perm;
> +        const char *name;
> +    } permissions[] = {
> +        { BLK_PERM_CONSISTENT_READ, "consistent read" },
> +        { BLK_PERM_WRITE,           "write" },
> +        { BLK_PERM_WRITE_UNCHANGED, "write unchanged" },
> +        { BLK_PERM_RESIZE,          "resize" },
> +        { BLK_PERM_GRAPH_MOD,       "change children" },
> +        { 0, NULL }
> +    };
> +
> +    char *result = g_strdup("");
> +    struct perm_name *p;
> +
> +    for (p = permissions; p->name; p++) {
> +        if (perm & p->perm) {
> +            char *old = result;
> +            result = g_strdup_printf("%s%s%s", old, *old ? ", " : "", p->name);
> +            g_free(old);
> +        }
> +    }
> +
> +    return result;
> +}
> +
>  static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
>                                    uint64_t new_shared_perm,
>                                    BdrvChild *ignore_child, Error **errp)
> @@ -1397,17 +1434,24 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
>              continue;
>          }
>  
> -        if ((new_used_perm & c->shared_perm) != new_used_perm ||
> -            (c->perm & new_shared_perm) != c->perm)
> -        {
> -            const char *user = NULL;
> -            if (c->role->get_name) {
> -                user = c->role->get_name(c);
> -                if (user && !*user) {
> -                    user = NULL;
> -                }
> -            }
> -            error_setg(errp, "Conflicts with %s", user ?: "another operation");
> +        if ((new_used_perm & c->shared_perm) != new_used_perm) {
> +            char *link = bdrv_child_link_name(c);
> +            char *perm_names = bdrv_perm_names(new_used_perm & ~c->shared_perm);
> +            error_setg(errp, "Conflicts with %s, which does not allow '%s' "
> +                             "on %s",
> +                       link, perm_names, bdrv_get_node_name(c->bs));
> +            g_free(link);
> +            g_free(perm_names);
> +            return -EPERM;
> +        }
> +
> +        if ((c->perm & new_shared_perm) != c->perm) {
> +            char *link = bdrv_child_link_name(c);
> +            char *perm_names = bdrv_perm_names(c->perm & ~new_shared_perm);
> +            error_setg(errp, "Conflicts with %s, which uses '%s' on %s",
> +                       link, perm_names, bdrv_get_node_name(c->bs));
> +            g_free(link);
> +            g_free(perm_names);
>              return -EPERM;
>          }
>  
>