Re: [PATCH v2 2/4] blkdebug: Allow taking/unsharing permissions

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

23.09.2019 14:57, Max Reitz wrote:
> Sometimes it is useful to be able to add a node to the block graph that
> takes or unshare a certain set of permissions for debugging purposes.
> This patch adds this capability to blkdebug.
> 
> (Note that you cannot make blkdebug release or share permissions that it
> needs to take or cannot share, because this might result in assertion
> failures in the block layer.  But if the blkdebug node has no parents,
> it will not take any permissions and share everything by default, so you
> can then freely choose what permissions to take and share.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qapi/block-core.json |  14 +++++-
>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5cd00c361..572c5756f1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3394,6 +3394,16 @@
>   #
>   # @set-state:       array of state-change descriptions
>   #
> +# @take-child-perms: Permissions to take on @image in addition to what
> +#                    is necessary anyway (which depends on how the
> +#                    blkdebug node is used).  Defaults to none.
> +#                    (since 4.2)
> +#
> +# @unshare-child-perms: Permissions not to share on @image in addition
> +#                       to what cannot be shared anyway (which depends
> +#                       on how the blkdebug node is used).  Defaults
> +#                       to none.  (since 4.2)
> +#
>   # Since: 2.9
>   ##
>   { 'struct': 'BlockdevOptionsBlkdebug',
> @@ -3403,7 +3413,9 @@
>               '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
>               '*opt-discard': 'int32', '*max-discard': 'int32',
>               '*inject-error': ['BlkdebugInjectErrorOptions'],
> -            '*set-state': ['BlkdebugSetStateOptions'] } }
> +            '*set-state': ['BlkdebugSetStateOptions'],
> +            '*take-child-perms': ['BlockPermission'],
> +            '*unshare-child-perms': ['BlockPermission'] } }
>   
>   ##
>   # @BlockdevOptionsBlklogwrites:
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5ae96c52b0..f3c1e4ad7b 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -28,9 +28,11 @@
>   #include "qemu/cutils.h"
>   #include "qemu/config-file.h"
>   #include "block/block_int.h"
> +#include "block/qdict.h"
>   #include "qemu/module.h"
>   #include "qemu/option.h"
>   #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qlist.h"
>   #include "qapi/qmp/qstring.h"
>   #include "sysemu/qtest.h"
>   
> @@ -44,6 +46,9 @@ typedef struct BDRVBlkdebugState {
>       uint64_t opt_discard;
>       uint64_t max_discard;
>   
> +    uint64_t take_child_perms;
> +    uint64_t unshare_child_perms;
> +
>       /* For blkdebug_refresh_filename() */
>       char *config_file;
>   
> @@ -344,6 +349,84 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
>       qdict_put_str(options, "x-image", filename);
>   }
>   
> +static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
> +                                    const char *prefix, Error **errp)
> +{
> +    int ret = 0;
> +    QDict *subqdict = NULL;
> +    QObject *crumpled_subqdict = NULL;
> +    QList *perm_list;
> +    const QListEntry *perm;
> +
> +    qdict_extract_subqdict(options, &subqdict, prefix);
> +    if (!qdict_size(subqdict)) {
> +        goto out;
> +    }
> +
> +    crumpled_subqdict = qdict_crumple(subqdict, errp);
> +    if (!crumpled_subqdict) {
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    perm_list = qobject_to(QList, crumpled_subqdict);
> +    if (!perm_list) {
> +        /* Omit the trailing . from the prefix */
> +        error_setg(errp, "%.*s expects a list",
> +                   (int)(strlen(prefix) - 1), prefix);
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +
> +    for (perm = qlist_first(perm_list); perm; perm = qlist_next(perm)) {
> +        const char *perm_name;
> +        BlockPermission perm_bit;
> +
> +        perm_name = qstring_get_try_str(qobject_to(QString, perm->value));
> +        if (!perm_name) {
> +            /* Omit the trailing . from the prefix */
> +            error_setg(errp, "%.*s expects a list of enum strings",
> +                       (int)(strlen(prefix) - 1), prefix);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        perm_bit = qapi_enum_parse(&BlockPermission_lookup, perm_name,
> +                                   BLOCK_PERMISSION__MAX, errp);
> +        if (perm_bit == BLOCK_PERMISSION__MAX) {
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        *dest |= UINT64_C(1) << perm_bit;
> +    }


I still think that parsing it all by hand is a bad idea. We already have
functions which does it, why to opencode? The following works for me:

static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
                                     const char *prefix, Error **errp)
{
     Error *local_err = NULL;
     Visitor *v = NULL;
     BlockPermissionList *list = NULL, *perm;
     int ret = 0;
     QDict *subqdict = NULL;
     QObject *crumpled_subqdict = NULL;
     uint64_t perm_map[] = {
         [BLOCK_PERMISSION_CONSISTENT_READ] = BLK_PERM_CONSISTENT_READ,
         [BLOCK_PERMISSION_WRITE] = BLK_PERM_WRITE,
         [BLOCK_PERMISSION_WRITE_UNCHANGED] = BLK_PERM_WRITE_UNCHANGED,
         [BLOCK_PERMISSION_RESIZE] = BLK_PERM_RESIZE,
         [BLOCK_PERMISSION_GRAPH_MOD] = BLK_PERM_GRAPH_MOD,
     };

     qdict_extract_subqdict(options, &subqdict, prefix);
     if (!qdict_size(subqdict)) {
         goto out;
     }

     crumpled_subqdict = qdict_crumple(subqdict, errp);
     if (!crumpled_subqdict) {
         ret = -EINVAL;
         goto out;
     }

     v = qobject_input_visitor_new(QOBJECT(crumpled_subqdict));
     visit_type_BlockPermissionList(v, NULL, &list, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }

     for (perm = list; perm; perm = perm->next) {
         *dest |= perm_map[perm->value];
     }

     qapi_free_BlockPermissionList(list);

out:
     visit_free(v);
     qobject_unref(subqdict);
     qobject_unref(crumpled_subqdict);
     return ret;
}


Probably, we can do similar thing for the whole blkdebug_open and drop runtime_opts,
but it's for another series..

> +
> +out:
> +    qobject_unref(subqdict);
> +    qobject_unref(crumpled_subqdict);
> +    return ret;
> +}
> +
> +static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
> +                                Error **errp)
> +{
> +    int ret;
> +
> +    ret = blkdebug_parse_perm_list(&s->take_child_perms, options,
> +                                   "take-child-perms.", errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = blkdebug_parse_perm_list(&s->unshare_child_perms, options,
> +                                   "unshare-child-perms.", errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>   static QemuOptsList runtime_opts = {
>       .name = "blkdebug",
>       .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> @@ -419,6 +502,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>       /* Set initial state */
>       s->state = 1;
>   
> +    /* Parse permissions modifiers before opening the image file */
> +    ret = blkdebug_parse_perms(s, options, errp);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>       /* Open the image file */
>       bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
>                                  bs, &child_file, false, &local_err);
> @@ -916,6 +1005,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
>       return 0;
>   }
>   
> +static void blkdebug_child_perm(BlockDriverState *bs, BdrvChild *c,
> +                                const BdrvChildRole *role,
> +                                BlockReopenQueue *reopen_queue,
> +                                uint64_t perm, uint64_t shared,
> +                                uint64_t *nperm, uint64_t *nshared)
> +{
> +    BDRVBlkdebugState *s = bs->opaque;
> +
> +    bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared,
> +                              nperm, nshared);
> +
> +    *nperm |= s->take_child_perms;
> +    *nshared &= ~s->unshare_child_perms;
> +}
> +
>   static const char *const blkdebug_strong_runtime_opts[] = {
>       "config",
>       "inject-error.",
> @@ -940,7 +1044,7 @@ static BlockDriver bdrv_blkdebug = {
>       .bdrv_file_open         = blkdebug_open,
>       .bdrv_close             = blkdebug_close,
>       .bdrv_reopen_prepare    = blkdebug_reopen_prepare,
> -    .bdrv_child_perm        = bdrv_filter_default_perms,
> +    .bdrv_child_perm        = blkdebug_child_perm,
>   
>       .bdrv_getlength         = blkdebug_getlength,
>       .bdrv_refresh_filename  = blkdebug_refresh_filename,
> 
kkkk

-- 
Best regards,
Vladimir