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 | 29 +++++++++++-
block/blkdebug.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 133 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e6edd641f1..336043e02c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3347,6 +3347,21 @@
'*state': 'int',
'new_state': 'int' } }
+##
+# @BlockdevPermission:
+#
+# Permissions that an edge in the block graph can take or share.
+#
+# Since: 4.2
+##
+{ 'enum': 'BlockdevPermission',
+ 'data': [
+ 'consistent-read',
+ 'write',
+ 'write-unchanged',
+ 'resize',
+ 'graph-mod' ] }
+
##
# @BlockdevOptionsBlkdebug:
#
@@ -3388,6 +3403,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',
@@ -3397,7 +3422,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': ['BlockdevPermission'],
+ '*unshare-child-perms': ['BlockdevPermission'] } }
##
# @BlockdevOptionsBlklogwrites:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..ec24a5e4e5 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;
+ BlockdevPermission 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(&BlockdevPermission_lookup, perm_name,
+ BLOCKDEV_PERMISSION__MAX, errp);
+ if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ *dest |= UINT64_C(1) << perm_bit;
+ }
+
+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,
--
2.21.0
12.09.2019 16:56, 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 | 29 +++++++++++-
> block/blkdebug.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 133 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e6edd641f1..336043e02c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3347,6 +3347,21 @@
> '*state': 'int',
> 'new_state': 'int' } }
>
> +##
> +# @BlockdevPermission:
> +#
> +# Permissions that an edge in the block graph can take or share.
> +#
> +# Since: 4.2
> +##
> +{ 'enum': 'BlockdevPermission',
> + 'data': [
> + 'consistent-read',
> + 'write',
> + 'write-unchanged',
> + 'resize',
> + 'graph-mod' ] }
:)
BlockPermission is already here since 4.0 and has exactly same content. (And better documented)
> +
> ##
> # @BlockdevOptionsBlkdebug:
> #
> @@ -3388,6 +3403,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',
> @@ -3397,7 +3422,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': ['BlockdevPermission'],
> + '*unshare-child-perms': ['BlockdevPermission'] } }
>
> ##
> # @BlockdevOptionsBlklogwrites:
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5ae96c52b0..ec24a5e4e5 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;
> + BlockdevPermission 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(&BlockdevPermission_lookup, perm_name,
> + BLOCKDEV_PERMISSION__MAX, errp);
> + if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *dest |= UINT64_C(1) << perm_bit;
> + }
> +
> +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;
> +}
It's a pity that being described in json, these new parameters still not parsed automatically..
> +
> static QemuOptsList runtime_opts = {
and that we have to keep these runtime_opts everywhere, which duplicates json definitions..
> .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,
>
--
Best regards,
Vladimir
On 18.09.19 18:01, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, 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 | 29 +++++++++++-
>> block/blkdebug.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 133 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e6edd641f1..336043e02c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3347,6 +3347,21 @@
>> '*state': 'int',
>> 'new_state': 'int' } }
>>
>> +##
>> +# @BlockdevPermission:
>> +#
>> +# Permissions that an edge in the block graph can take or share.
>> +#
>> +# Since: 4.2
>> +##
>> +{ 'enum': 'BlockdevPermission',
>> + 'data': [
>> + 'consistent-read',
>> + 'write',
>> + 'write-unchanged',
>> + 'resize',
>> + 'graph-mod' ] }
>
> :)
>
> BlockPermission is already here since 4.0 and has exactly same content. (And better documented)
Oops. Very good, then, thanks. :-)
[...]
>> @@ -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;
>> + BlockdevPermission 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(&BlockdevPermission_lookup, perm_name,
>> + BLOCKDEV_PERMISSION__MAX, errp);
>> + if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + *dest |= UINT64_C(1) << perm_bit;
>> + }
>> +
>> +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;
>> +}
>
>
> It's a pity that being described in json, these new parameters still not parsed automatically..
That would require changing the whole bdrv_open() infrastructure to use
QAPI types.
>> +
>> static QemuOptsList runtime_opts = {
>
> and that we have to keep these runtime_opts everywhere, which duplicates json definitions..
Well, it duplicates some json definitions. I don’t add the new
parameters added in this patch to the list, because there is no point in
supporting them outside of blockdev-add.
Max
© 2016 - 2025 Red Hat, Inc.