14.10.2019 18:39, 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 <mre...@redhat.com> > --- > qapi/block-core.json | 14 ++++++- > block/blkdebug.c | 91 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 103 insertions(+), 2 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index f66553aac7..054ce651de 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3453,6 +3453,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', > @@ -3462,7 +3472,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..6807c03065 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -28,10 +28,14 @@ > #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/qapi-visit-block-core.h" > #include "qapi/qmp/qdict.h" > +#include "qapi/qmp/qlist.h" > #include "qapi/qmp/qstring.h" > +#include "qapi/qobject-input-visitor.h" > #include "sysemu/qtest.h" > > typedef struct BDRVBlkdebugState { > @@ -44,6 +48,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 +351,67 @@ 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; > + Visitor *v = NULL; > + BlockPermissionList *perm_list = NULL, *element; > + Error *local_err = NULL; > + > + qdict_extract_subqdict(options, &subqdict, prefix); > + if (!qdict_size(subqdict)) {
Hmm, you consider it as a success, so you mean default. Then, it's safer to set *dest = 0 here. > + goto out; > + } > + > + crumpled_subqdict = qdict_crumple(subqdict, errp); > + if (!crumpled_subqdict) { > + ret = -EINVAL; > + goto out; > + } > + > + v = qobject_input_visitor_new(crumpled_subqdict); > + visit_type_BlockPermissionList(v, NULL, &perm_list, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto out; > + } > + I'd prefer explicitly set *dest = 0 here too. > + for (element = perm_list; element; element = element->next) { > + *dest |= UINT64_C(1) << element->value; Hmm, so, you rely on correct correspondence between generated BlockPermission enum and unnamed enum with BLK_PERM_* constants... I'm afraid it's unsafe, so, in xdbg_graph_add_edge() special mapping variable is used + QEMU_BUILD_BUG_ON on BLK_PERM_ALL. I think something like this should be done here. > + } > + > +out: > + qapi_free_BlockPermissionList(perm_list); > + visit_free(v); > + 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 +487,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 +990,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 +1029,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, > -- Best regards, Vladimir