Re: [Qemu-block] [RFC PATCH 23/41] block: Include details on permission errors in message

[Max Reitz]

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 <address@hidden>
> ---
>  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;
>          }
>  
>

signature.asc
Description: OpenPGP digital signature